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.

Sign up for GitHub

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

Jump to bottom

Improve autoupdate round 2 #37

Merged
merged 5 commits into from
Sep 24, 2024
Merged

Improve autoupdate round 2 #37

merged 5 commits into from
Sep 24, 2024

Conversation

lldelisle
Copy link
Contributor

Looking at the logs there are at least 2 things to fix:

  • deal with cases when multiple PR titles match
  • clean repo if the current PR has the same changes as after autoupdate.

nsoranzo
nsoranzo reviewed Sep 24, 2024
View reviewed changes
.github/workflows/autoupdate.yml Outdated Show resolved Hide resolved
.github/workflows/autoupdate.yml Outdated Show resolved Hide resolved
.github/workflows/autoupdate.yml Outdated Show resolved Hide resolved
.github/workflows/autoupdate.yml Outdated Show resolved Hide resolved
.github/workflows/autoupdate.yml Outdated Show resolved Hide resolved
@lldelisle
Copy link
Contributor Author

@nsoranzo I added this head -n 1 because you can have multiple closed PR from the same branch, see bgruening/galaxytools#1353 and bgruening/galaxytools#1350.
This is what caused the CI to fail for bgruening galaxytools repo: https://github.com/planemo-autoupdate/autoupdate/actions/runs/10995709180/job/30537933271#step:10:1069

But probably the head -n 1 is only useful for closed PR.

@lldelisle @nsoranzo
Remove unnecessary head -n 1
aae66a0 
Co-authored-by: Nicola Soranzo <[email protected]>
@nsoranzo
Copy link
Collaborator

Yes, I think you missed my suggestion to add | head -n 1 at line 127.

@nsoranzo nsoranzo closed this Sep 24, 2024
@nsoranzo nsoranzo reopened this Sep 24, 2024
@lldelisle
Copy link
Contributor Author

Indeed, I just fixed this.

@lldelisle
Copy link
Contributor Author

Sorry...

@nsoranzo nsoranzo merged commit 2d82800 into planemo-autoupdate:main Sep 24, 2024
@nsoranzo
Copy link
Collaborator

Thanks @lldelisle !

@lldelisle
Copy link
Contributor Author

This will probably need a round 3 but thanks @nsoranzo for the review. I feel like we are moving forward.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nsoranzo nsoranzo nsoranzo approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@lldelisle @nsoranzo