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

Add action to automatically update PRs with tox -e lint #2369

Open
matentzn opened this issue May 17, 2023 · 2 comments · May be fixed by #2412
Open

Add action to automatically update PRs with tox -e lint #2369

matentzn opened this issue May 17, 2023 · 2 comments · May be fixed by #2412
Assignees
Labels
attn: Technical WG Issues pertinent to technical activities, such as maintenance of website, PURLs, and tools pipeline Issues related to development or revision of global Foundry pipelines

Comments

@matentzn
Copy link
Contributor

Its annoying to do this manually - this should be done automatically.

The github workflows in this repo have enough detail to figure out how to do this probably.

Here is an example of how in OBA we commit changes to a file using GH action: https://github.com/obophenotype/bio-attribute-ontology/blob/master/.github/workflows/dosdp.yml#L26

@erik-whiting
Copy link
Contributor

Are you saying you want tox run on any pull requests once they're submitted? If so, I have a suggestion, because there's a small problem that will come with that.

Say you submit a PR, and this worker automatically runs tox. The worker then commits the changes to the PR to save them. Now say someone asks you to change something on your PR. You make the small change locally and then try to push, but git rejects your push because your local branch is now out of sync with the remote branch.

As soon as any worker makes a commit on a remote branch, all local branches tracking it are now out of sync. You have to have the wherewithal to make sure you pull the upstream branch before making your changes. You could of course do a git push --force as the error message will likely suggest you do, or you could stash your changes, pull, and then unstash the changes once yoru local is resynced with remote. You could also delete your local branch and re-pull the remote branch.

For you and me, these are likely non-issues as we are quite familiar with git and have probably resynced our local branches 1,000 times this year alone. But I think not everyone involved in this project is so comfortable with git, and a warning message telling someone to use a flag called "force" is probably scary. For this reason, I don't really like this suggestion.

What I suggest instead is running tox and applying the changes immediately before merge. That is, once a PR has been approved and we click merge, the CI runs tox -e lint, commits the changes if there are any, and then goes on with the rest of its workflow. That way, the changes are already approved and that branch is about to be useless anyway, so no need to worry about out-of-sync branches.

How do you feel about this instead?

@matentzn
Copy link
Contributor Author

This would mean, however, that the lint check (not the operation of applying the linting, but the test that checks if the file is well formatted) cant happen.. Will it not be exceedingly difficult to separate formatting issues that will be solved by running tox -e lint from the ones that won't be fixed by it?

I feel like 99% of the people use the GitHub user interface to change metadata. They wouldn't notice the updated branch.

The best would be a more deliberate "fix formatting issues" button that could be run by the person submitting the updated file, but I don't think that is possible..

@nlharris nlharris added the attn: Technical WG Issues pertinent to technical activities, such as maintenance of website, PURLs, and tools label May 19, 2023
@nlharris nlharris added the pipeline Issues related to development or revision of global Foundry pipelines label May 29, 2023
@erik-whiting erik-whiting linked a pull request Jul 29, 2023 that will close this issue
@matentzn matentzn assigned jsstevenson and unassigned erik-whiting Apr 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
attn: Technical WG Issues pertinent to technical activities, such as maintenance of website, PURLs, and tools pipeline Issues related to development or revision of global Foundry pipelines
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants