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

release-notes (and release-sdk) don't check the remotes when reusing an existing temp dir for repos #3444

Closed
jeremyrickard opened this issue Feb 2, 2024 · 8 comments · Fixed by #3458
Labels
area/release-eng Issues or PRs related to the Release Engineering subproject kind/bug Categorizes issue or PR as related to a bug. needs-priority sig/release Categorizes an issue or PR as relevant to SIG Release.

Comments

@jeremyrickard
Copy link
Contributor

What happened:

When running release-notes with start-rev and end-rev provided, it was observed that the wrong repo was being cloned/updated. The tooling was using kubernetes/kubernetes instead kubernetes-sigs/promo-tools. This appears to be happening because there is an existing repo clone in the expected temp directory, however it's for another repository (i.e. k/k). The cloneOrOpenRepo function in release-sdk doesn't ensure that the repoPath given is one of the remotes.

Slack Thread For Reference

What you expected to happen:

release-notes either provides a descriptive error and directions to clean up the temp dir or it clones to a new temp dir.

How to reproduce it (as minimally and precisely as possible):

Run 'release-notes' with no temp directory on one project. Then run it again on a second project (i.e. it would have a different repo path).

Anything else we need to know?:

Environment:

  • Cloud provider or hardware configuration:
  • OS (e.g: cat /etc/os-release):
  • Kernel (e.g. uname -a):
  • Others:
@jeremyrickard jeremyrickard added kind/bug Categorizes issue or PR as related to a bug. sig/release Categorizes an issue or PR as relevant to SIG Release. area/release-eng Issues or PRs related to the Release Engineering subproject labels Feb 2, 2024
@jeremyrickard
Copy link
Contributor Author

@DannyBrito and I just debugged this and we have a solution and will open a PR but we wanted to chat about it first. I think the proper fix here is to update release-sdk to make cloneOrOpenRepo check that the remotes for the specified repo contain the repoPath argument and return an error if it doesn't. Then we can either handle the error in release-notes and make it create a new temp dir and go forward, or maybe it would be more appropriate to just error and provide an error message indicating that the temp dir needs to be cleaned up.

This is probably going to impact very few people since most folks aren't running release-notes for multiple projects :)

@puerco @saschagrunert wdyt?

@cpanato
Copy link
Member

cpanato commented Feb 3, 2024

@jeremyrickard, what are the parameters that you use to run the rel-notes?

to run for another repo usually you do

release-notes --required-author="" --output=release-notes.md --start-rev v0.10.4 --end-rev v0.11.0 --org kubernetes-sigs --repo release-sdk --branch main

@xmudrii
Copy link
Member

xmudrii commented Feb 3, 2024

@cpanato I used the following command:

release-notes generate \
    --org="kubernetes-sigs" \
    --repo="promo-tools" \
    --start-rev="v4.0.4" \
    --end-rev="v4.0.5" \
    --branch="main" \
    --markdown-links

@jeremyrickard
Copy link
Contributor Author

@cpanato yeah the issue here using you already have a temp directory with the remotes pointing at another repo, we just reuse that dir and run a git pull —rebase. We don’t check that it’s the right repo

@cpanato
Copy link
Member

cpanato commented Feb 3, 2024

I see so the issue is we need to clean the temp dir as well

@saschagrunert
Copy link
Member

cloneOrOpenRepo check that the remotes for the specified repo contain the repoPath argument and return an error if it doesn't. Then we can either handle the error in release-notes and make it create a new temp dir and go forward, or maybe it would be more appropriate to just error and provide an error message indicating that the temp dir needs to be cleaned up.

Yeah checking the remote and wiping the cache if they don't match is a good solution. 👍

@cpanato
Copy link
Member

cpanato commented Feb 5, 2024

@jeremyrickard should I do the fix or you already have something?

@jeremyrickard
Copy link
Contributor Author

@cpanato have a fix. we'll open a PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/release-eng Issues or PRs related to the Release Engineering subproject kind/bug Categorizes issue or PR as related to a bug. needs-priority sig/release Categorizes an issue or PR as relevant to SIG Release.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants