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

Update dev version with GHA #455

Closed
wants to merge 17 commits into from
Closed

Update dev version with GHA #455

wants to merge 17 commits into from

Conversation

savente93
Copy link
Contributor

@savente93 savente93 commented Jul 25, 2023

Issue addressed

Fixes #173

Explanation

EDIT: explanation below is just left here for posterity. Explanation of new implementation in comments below!
Quite a simple implementation really. Since the version is dynamically set anyway, I figured to solve the issue, we might as well just parse the git history in situ to determine the version. Technically, this means that git must be installed to use hydromt properly, but given all of the workflows I'm familiar with I don't think this is a controversial assumption. Just in case I also added a fallback mechanism which will just fall back to the base version.

The upside of this implementation as to setting the version statically and, for example using GHA to set it accordingly is that it's more informative (though not definitive) before the commit lands in main, as well as continues to work across history rewrites. Technically this could break reproducibility if we decide to rewrite main, but that is generally it's own hole can of head ache and should generally be avoided, so I think this should be okay for now.

Decided not to add tests for this, since git is a very controlled and stable interface, and it's quite a small PR for this. If reviewers want to see tests anyway, feel free to let me know, and I'll add them.

Checklist

  • Updated tests or added new tests
  • Branch is up to date with main
  • Tests & pre-commit hooks pass
  • Updated documentation if needed
  • Updated changelog.rst if needed

Additional Notes (optional)

Add any additional notes or information that may be helpful.

@savente93 savente93 marked this pull request as ready for review July 25, 2023 11:13
@savente93 savente93 marked this pull request as draft July 25, 2023 12:46
@savente93
Copy link
Contributor Author

turns out this approach doesn't work when trying to run pip install . so I'll have to think of something else. so converted back to draft

@savente93 savente93 changed the title dynamicaly determin version based on git history Update dev version with GHA Jul 25, 2023
@savente93
Copy link
Contributor Author

Based on the previous breakage, this is more or less the only way to do it. Now the workflow is, that once someone approves a PR, (which is necessary for merging to main anyway) A bot will add a new commit to your branch that updates the version accordingly. if this gets merged it will be very important that everybody merges to main with Squash commits! there are settings for this in the repo, I'll discuss with an admin when I can. ping me if you want an explanation about what this means. I've also updated the documentation to reflect this.

@savente93
Copy link
Contributor Author

in the fork that I used for testing, I couldn't figure out how to get the approved PR event to trigger, so I haven't tested that branch, but at least the manual branch works. I'll do some more PR approval testing tomorrow

@savente93
Copy link
Contributor Author

Tested the trigger in a dummy repo, it worked fine, should all be good to go!

@savente93 savente93 marked this pull request as ready for review July 26, 2023 10:31
Copy link
Contributor

@DirkEilander DirkEilander left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working this out! I agree with the solution to not derive the version on runtime. However, I wonder if it wouldn't be more robust to base the next version on the current version in __init__.py rather than the number of commits since the last release. Would this also be possible?

@savente93
Copy link
Contributor Author

@DirkEilander indeed it is, I've made the change, should be good now. I'm moderately confident it is correct but didn't do the off-screen testing because it's kind of laborious, Do you want me to rerun that?

Copy link
Contributor

@DirkEilander DirkEilander left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me! Approving this PR would be the proof of the pudding correct?

Copy link
Contributor

@DirkEilander DirkEilander left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@savente93 unfortunately the action fails.. Would a simple python script to do the job make it easier? Or is the failure because of another step in the process? And what would actually be the behaviour if the current version is "v0.8.0" without "devX"?

@savente93
Copy link
Contributor Author

It's not actually the script that's failing but the notification mechanism. I don't think there are good ways of doing that in python, besides that would add a whole different headache with installing and versioning python, so I don't think adding python will help. I'm happy to walk you through/document what the code does. Sadly the bit that is to do with interacting with GitHub isn't super well documented so that' why that failed. I'll fix it today, shouldn't be too big of a problem

As for what it does if the .devX part isn't there, I didn't include it before because I thought adding that back was part of the release process but I agree it might be better to have that. Should be good now.

Copy link
Contributor

@DirkEilander DirkEilander left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm really exited about this new automation step, cool stuff!
Before merging there are two points which perhaps would be easiest to discuss briefly in the office next week.

  • The workflow is now triggered by approved review . Could this still lead to incorrect versioning if the version number in the branch is not up to date at the moment of merging? Or would merging conflicts prevent this going wrong? Alternatively, would it also be possible to trigger this based on merge commits to the main branch and could that avoid potential merge conflicts? Happy to follow your experience here.
  • How does this affect the publishing workflow? For a release we will still manually set the release version in a branch and create a tag for this version. When we then merge this branch to the main the version number will have been modified to the next dev version. The release is still based on the tagged version with correct version number, so I guess that's fine. However, the docs are currently only generated after merging to main which means that only dev docs will be published, correct? Could we fix this by adding a trigger to publish docs based on the release or tag?

@savente93
Copy link
Contributor Author

Sure let's discuss on Tuesday

@savente93
Copy link
Contributor Author

I was actually curious if the scenario you presented would create a merge conflict, and while setting up that test, I found a few other problems that I'm working on fixing right now. fixes should be ready EOD. We'll still discuss tomorrow but for both austerities sake and as a primer:

The workflow is now triggered by approved review . Could this still lead to incorrect versioning if the version number in the branch is not up to date at the moment of merging? Or would merging conflicts prevent this going wrong?

Yes this can lead to incorrect versioning if something else gets merged first, though I do have an idea for a workflow that can fix this. Basically there is a setting in GH that says something to the effect of "require branch up to date before merging". I can then add a required action that checks if the versions are updated/the same as main (which they should not be in the branch). This would prevent any inconsistent versioning, because people would have to update before being allowed to merge and then the action would ensure consistent versions. The downside to this is that you have to manually update your branches when you're merging multiple, but I do that anyway since I want to be sure tests will pass before merging, so I actually think this is fine.

Alternatively, would it also be possible to trigger this based on merge commits to the main branch and could that avoid potential merge conflicts?

Technically yes, that is possible, however since after a merge commit has happened, the PR is no longer considered active by github. This means that the action would have to write straight to main, and because of the write protection, the action would have to bypass a whole lot of safety checks to do it, and I do not trust myself enough to be correct to do that, so imo this direction is a no-go, which is why I went for the current approach.

As an addendum to this, you can re-update the version by simply re approving the commit, but that can get a bit annoying, so I'll see if I can add a manual way to rerun it so if it does get out of sync, the author can rerun it themselves.

How does this affect the publishing workflow? For a release we will still manually set the release version in a branch and create a tag for this version. When we then merge this branch to the main the version number will have been modified to the next dev version. The release is still based on the tagged version with correct version number, so I guess that's fine. However, the docs are currently only generated after merging to main which means that only dev docs will be published, correct? Could we fix this by adding a trigger to publish docs based on the release or tag?

I'm not quite sure I follow the question, but creating a publish docs based on release or tag is definitely possible. We'll discuss further tomorrow

@savente93 savente93 marked this pull request as draft August 2, 2023 09:17
@savente93 savente93 closed this Aug 8, 2023
@savente93
Copy link
Contributor Author

Blocked by a bug in the github authentication code for github apps. Deleting this branch since the up to date code is on the automation demo repo where I was testing it

@savente93 savente93 deleted the auto-version branch August 8, 2023 15:16
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.

use bump2version for versioning
2 participants