Skip to content

move testutils under an ./internal/ directory#601

Merged
rdimitrov merged 1 commit intotheupdateframework:masterfrom
mikedanese:internal
Feb 6, 2024
Merged

move testutils under an ./internal/ directory#601
rdimitrov merged 1 commit intotheupdateframework:masterfrom
mikedanese:internal

Conversation

@mikedanese
Copy link
Copy Markdown
Contributor

This prevents these libraries from being depended on from outside the module.

@rdimitrov
Copy link
Copy Markdown
Contributor

@mikedanese - there's some things to fix so we can satisfy the CI but I agree it's better if these packages are internal 👍 Thanks for addressing this 🙏

@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Feb 2, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (3a2b819) 70.18% compared to head (a1b3a1f) 70.18%.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #601   +/-   ##
=======================================
  Coverage   70.18%   70.18%           
=======================================
  Files          10       10           
  Lines        2123     2123           
=======================================
  Hits         1490     1490           
  Misses        517      517           
  Partials      116      116           
Flag Coverage Δ
Go-1.21 70.18% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mikedanese
Copy link
Copy Markdown
Contributor Author

Oops, fixed.

rdimitrov
rdimitrov previously approved these changes Feb 2, 2024
@codysoyland
Copy link
Copy Markdown
Contributor

FWIW, I was looking at using the RepositorySimulator in testutils as part of sigstore-go's test suite, but I can understand why it's probably best to keep it internal.

@rdimitrov
Copy link
Copy Markdown
Contributor

rdimitrov commented Feb 5, 2024

@codysoyland - oh, I wasn't aware of that, thanks 👍 Given this I think it's okay to leave it as a standalone package. cc: @mikedanese

edit:
@codysoyland - hey, I just saw you mentioned in the PR that eventually you came up with another workaround for the tests. I don't think there's anyone else trying to use the testutils package so far, but I'm supportive of leaving it visible if it's going to help sigstore-go with the testing.

Of course, at the end we can always revisit and move it out of internal if someone comes up with a valid use case for it 👍

@codysoyland
Copy link
Copy Markdown
Contributor

edit: @codysoyland - hey, I just saw you mentioned in the PR that eventually you came up with another workaround for the tests. I don't think there's anyone else trying to use the testutils package so far, but I'm supportive of leaving it visible if it's going to help sigstore-go with the testing.

Of course, at the end we can always revisit and move it out of internal if someone comes up with a valid use case for it 👍

@rdimitrov Thanks for offering to keep it visible, but I decided late yesterday that it makes sense for us to have our own stripped down version of a repo simulator and not depend on the one here, since it may diverge and become incompatible, so I'm supportive of going ahead and merging this one!

@rdimitrov
Copy link
Copy Markdown
Contributor

@codysoyland - Thanks! 😃👍

@mikedanese - There's a few conflicts you have to fix, but otherwise we should be good to go then 👍

This prevents these libraries from being depended on from outside the
module.

Signed-off-by: Mike Danese <mike.danese@databricks.com>
@mikedanese
Copy link
Copy Markdown
Contributor Author

Rebased.

Copy link
Copy Markdown
Contributor

@rdimitrov rdimitrov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The failing CI job is not related to the changes in this PR (tries to run go 1.22 and apparently something is not supporting it yet).

@rdimitrov rdimitrov merged commit 85bd220 into theupdateframework:master Feb 6, 2024
@mikedanese mikedanese deleted the internal branch February 9, 2024 00:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants