Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

move testutils under an ./internal/ directory #601

Merged
merged 1 commit into from
Feb 6, 2024

Conversation

mikedanese
Copy link
Contributor

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

@rdimitrov
Copy link
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

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
Contributor Author

Oops, fixed.

rdimitrov
rdimitrov previously approved these changes Feb 2, 2024
@codysoyland
Copy link
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
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
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
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
Contributor Author

Rebased.

Copy link
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
20 of 21 checks passed
@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