theupdateframework / go-tuf

Go implementation of The Update Framework (TUF)
https://theupdateframework.com
Apache License 2.0
625 stars 105 forks source link

move testutils under an ./internal/ directory #601

Closed mikedanese closed 7 months ago

mikedanese commented 8 months ago

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

rdimitrov commented 8 months ago

@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 commented 8 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

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

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

Additional details and impacted files ```diff @@ 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](https://app.codecov.io/gh/theupdateframework/go-tuf/pull/601/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=theupdateframework) | Coverage Ξ” | | |---|---|---| | [Go-1.21](https://app.codecov.io/gh/theupdateframework/go-tuf/pull/601/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=theupdateframework) | `70.18% <ΓΈ> (ΓΈ)` | | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=theupdateframework#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

mikedanese commented 8 months ago

Oops, fixed.

codysoyland commented 8 months ago

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 commented 8 months ago

@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 commented 7 months ago

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 commented 7 months ago

@codysoyland - Thanks! πŸ˜ƒπŸ‘

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

mikedanese commented 7 months ago

Rebased.