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

Fix subdirectory tagging #282

Merged
merged 2 commits into from
Sep 18, 2023

Conversation

sloede
Copy link
Contributor

@sloede sloede commented Sep 13, 2023

Fixes #281.

Note that this uses the suggestion by @ericphanson in #281 (comment) to proceed with tagging if the corresponding commit could be identified from the registry PR and if the subdirectory hash of that commit matches the Julia package version tree hash.

cc @bgeihe

@ericphanson
Copy link
Member

Is this something that we could easily add a test for? Or would that be a big effort?

@sloede
Copy link
Contributor Author

sloede commented Sep 15, 2023

Is this something that we could easily add a test for? Or would that be a big effort?

I am not sure how I would easily add a test for it, to be honest.

Any test would probably need to go into

def test_commit_sha_from_registry_pr(logger):
r = _repo()
r._registry_pr = Mock(return_value=None)
assert r._commit_sha_from_registry_pr("v1.2.3", "abc") is None
logger.info.assert_called_with("Did not find registry PR")
r._registry_pr.return_value = Mock(body="")
assert r._commit_sha_from_registry_pr("v2.3.4", "bcd") is None
logger.info.assert_called_with("Registry PR body did not match")
r._registry_pr.return_value.body = f"foo\n- Commit: {'a' * 32}\nbar"
r._repo.get_commit = Mock()
r._repo.get_commit.return_value.commit.tree.sha = "def"
r._repo.get_commit.return_value.sha = "sha"
assert r._commit_sha_from_registry_pr("v3.4.5", "cde") is None
r._repo.get_commit.assert_called_with("a" * 32)
logger.warning.assert_called_with(
"Tree SHA of commit from registry PR does not match"
)
assert r._commit_sha_from_registry_pr("v4.5.6", "def") == "sha"

What is hard to mock, however, is the _git.command(...). I checked other functions that directly run git commands, and, e.g., _versions_clone is also not tested explicitly as far as I can tell (since it would require a mock git repo somewhere with all the right ingredients. Beside this call to git, there is not really much else that could be checked.

Do have any other suggestions?

@ericphanson
Copy link
Member

No, I am unfortunately not familiar with the code either, but I totally believe it's hard to test. In that case since the feature is totally broken currently I think it makes sense to fix it still without adding a test. However it would be great if someone more familiar w/ the code could give it a review.

@sloede
Copy link
Contributor Author

sloede commented Sep 18, 2023

@christopher-dG it would be great if you could review this. I tested the code manually with our repo that originally caused this error to surface, and it seems like everything has been fixed. As stated above, adding tests for this is hard, since we would have to mock an entire git repo.

@christopher-dG
Copy link
Member

Seems good 👍

@christopher-dG christopher-dG merged commit 9807a75 into JuliaRegistries:master Sep 18, 2023
3 checks passed
@sloede sloede deleted the msl/fix-subdir-tagging branch September 18, 2023 17:59
@sloede
Copy link
Contributor Author

sloede commented Sep 19, 2023

Thanks a lot for for bearing with me, with TagBot v1.16.1 we finally have our first successful, TagBot-created release for the subdirectory package LibTrixi.jl (which is part of https://github.com/trixi-framework/libtrixi).

@ericphanson
Copy link
Member

Awesome! FYI, you can now configure Documenter to build stable docs based on tag prefixes, if you want to: https://documenter.juliadocs.org/dev/man/hosting/#Deploying-from-a-monorepo (another subdir feature thanks to @hannahilea!). Although I see you aren't using prefixes on your tags, so maybe that's irrelevant in this case.

@sloede
Copy link
Contributor Author

sloede commented Sep 19, 2023

You're right, we don't use prefixes, but thanks for letting me know 👍

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.

Subdirectory support broken due to wrong tree hash verification
3 participants