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

🎉 New release maker #1

Merged
merged 4 commits into from Jun 2, 2020
Merged

🎉 New release maker #1

merged 4 commits into from Jun 2, 2020

Conversation

fiendish
Copy link
Contributor

@fiendish fiendish commented May 18, 2020

Can't inline it, so I'll just link to the README.md


jobs:
context:
if: github.base_ref == 'master' && github.event.pull_request.merged && contains( github.event.pull_request.labels.*.name, 'release')
Copy link
Contributor

Choose a reason for hiding this comment

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

This requires a new release label? Is that preferable to a regex?

Copy link
Contributor Author

@fiendish fiendish May 19, 2020

Choose a reason for hiding this comment

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

I think it is probably preferable? Keep in mind that the label is added automatically by the user tool, so it's not like there's extra user effort involved.

We could use both to be double extra safe?

Copy link
Contributor Author

@fiendish fiendish May 19, 2020

Choose a reason for hiding this comment

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

I'm open to being convinced against using a label though. I don't have a strong feeling about best practice either way.

Copy link
Contributor

Choose a reason for hiding this comment

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

Label tag sound good because we currently label all PRs except for releases. We just need to make sure we have a standard release label across all repos.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I like the the label idea! Especially because the user doesn't have to remember to add it since the tool does it

# This workflow will install Python dependencies, run pre-release script, and
# then bundle a github release

name: Release generator
Copy link
Contributor

Choose a reason for hiding this comment

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

Fancy ⚡
It may be better to publish this as an action that way we can make updates to it without having to have everyone copy all the new files. See GH Docs on that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a good idea. I'll look into it.

Copy link
Contributor Author

@fiendish fiendish May 19, 2020

Choose a reason for hiding this comment

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

Upon reading up about actions/workflows/deployment, I don't think this will be feasible. I called this an action, but really it's a workflow, and a workflow doesn't only encode what happens but also when it happens, which a published action doesn't do, but which is very important for this to work right.

One way to provide centralized reusability could be to use git submodules instead of copying the file around.

- name: Create tag from title and run asset script
id: find_tag_and_prepare_assets
run: |
TAG=$(echo ${{ github.event.pull_request.title }} | sed -E "s/^.*Release (.+\..+\..+)$/\1/g")
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The PR titles should all be simpler than the full range of valid semver. But we can try it!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The official regex is pretty complex. As written, it breaks sed on macos. :\

Copy link
Contributor Author

@fiendish fiendish May 19, 2020

Choose a reason for hiding this comment

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

Alright, given that the CLI will only generate PRs with the the format 🔖 Release #.#.#, and that all this has to do is pick up the #.#.# part to turn it into a tag, which do you prefer:

# https://semver.org/#is-there-a-suggested-regular-expression-regex-to-check-a-semver-string
SEMVER="(0|[1-9]\d*)\.(0|[1-9]\d*)\.(0|[1-9]\d*)(?:-((?:0|[1-9]\d*|\d*[a-zA-Z-][0-9a-zA-Z-]*)(?:\.(?:0|[1-9]\d*|\d*[a-zA-Z-][0-9a-zA-Z-]*))*))?(?:\+([0-9a-zA-Z-]+(?:\.[0-9a-zA-Z-]+)*))?"
TAG=$(echo ${{ github.event.pull_request.title }} | perl -pe "s/^.*Release ${SEMVER}\$/\1.\2.\3/g")

or

TAG=$(echo ${{ github.event.pull_request.title }} | sed -E "s/^.*Release (.+\..+\..+)$/\1/g")

or something else?

Copy link
Collaborator

Choose a reason for hiding this comment

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

The only other thing I can think of that doesn't require a specific PR title format would be to have the CLI create a second label #.#.# which this workflow step could use and then delete after its finished?

This might also come in handy for the user provided script in step 8 of What the CLI Does. I'm thinking for most of the repos with Python packages, the user provided script will be used to update the package's __init__.py with the current version. For the kf-model-fhir repo, the user script will also need the version to in order to tag the FHIR model's resources with it.

Copy link
Contributor Author

@fiendish fiendish May 20, 2020

Choose a reason for hiding this comment

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

the user script will also need the version

Done in f4b2119

create a second label #.#.# which this workflow step could use and then delete after its finished

I like that. I'll see if it works and post that change if it does.
The downside is that it will pollute the label list if you use the CLI without using this GH action workflow. Thoughts on that?

Copy link
Collaborator

@znatty22 znatty22 May 20, 2020

Choose a reason for hiding this comment

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

Done in f4b2119

I think we're talking about two different user scripts. You're referring to the prepare assets script that gets run after the release and I'm referring to the pre-release script that gets executed in step 8 of release build right before the release branch is pushed up to Github.

Although, now that you mention the environment variable.. Maybe the CLI could write the version to a environment variable (e.g. GH_RELEASE_VERSION, something more specific than TAG) and then the pre-release script can read the version from there if it needs it.

If we can do this, then I'd say let's keep this workflow step (get tag) simple and stick with your first regex to get the version from the PR title?

Copy link
Contributor Author

@fiendish fiendish May 20, 2020

Choose a reason for hiding this comment

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

Both scripts are now passed the version as input. If they want to set environment variables, they can.

else:
return None

def _next_release_version(self, prev_version, release_type):
Copy link
Contributor

Choose a reason for hiding this comment

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

May be able to discard this in favor of just using semver's bump_major() style functions.

Copy link
Contributor Author

@fiendish fiendish May 19, 2020

Choose a reason for hiding this comment

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

I'm pretty sure I was using semver's bump functions in this code at some point. I wonder when that got flipped.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@@ -0,0 +1,59 @@
# This workflow will install Python dependencies, run pre-release script, and
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
# This workflow will install Python dependencies, run pre-release script, and
# This workflow will install Python dependencies, run prepare release asset script, and

@@ -0,0 +1,59 @@
# This workflow will load Python, run a pre-release script to generate assets,
Copy link
Collaborator

Choose a reason for hiding this comment

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

This file looks like a duplicate of .github/workflows/gh_release.yml?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

baleeted!

- name: Create tag from title and run asset script
id: find_tag_and_prepare_assets
run: |
TAG=$(echo ${{ github.event.pull_request.title }} | sed -E "s/^.*Release (.+\..+\..+)$/\1/g")
Copy link
Collaborator

Choose a reason for hiding this comment

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

The only other thing I can think of that doesn't require a specific PR title format would be to have the CLI create a second label #.#.# which this workflow step could use and then delete after its finished?

This might also come in handy for the user provided script in step 8 of What the CLI Does. I'm thinking for most of the repos with Python packages, the user provided script will be used to update the package's __init__.py with the current version. For the kf-model-fhir repo, the user script will also need the version to in order to tag the FHIR model's resources with it.

os.system(pre_release_script)

# Create and push new release branch
release_branch_name = f"🔖-release-{new_version}"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Whaaaat can git branch names have emojis 🙃?

Copy link
Contributor Author

@fiendish fiendish May 20, 2020

Choose a reason for hiding this comment

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

YUP! 😃

if pre_release_script:
print(f"Executing pre-release script {pre_release_script} ...")
os.chmod(pre_release_script, "u+x")
os.system(pre_release_script)
Copy link
Collaborator

@znatty22 znatty22 May 20, 2020

Choose a reason for hiding this comment

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

Will os.system raise an exception if something goes wrong? If not, maybe use subprocess? Whatever we use, as long as make_release fails if the pre-release script fails

Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess I have the same comment for the other os.system calls too. If they fail, will an exception be raised or will this script keep executing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok e39398f

Copy link
Collaborator

@znatty22 znatty22 left a comment

Choose a reason for hiding this comment

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

This is really cool! I tried it on one of my temp repos and it works really well. Just have one minor comment


if pre_release_script:
print(f"Executing pre-release script {pre_release_script} ...")
os.chmod(pre_release_script, "u+x")
Copy link
Collaborator

@znatty22 znatty22 May 21, 2020

Choose a reason for hiding this comment

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

Looks like os.chmod only takes the numeric form of the permission: https://docs.python.org/3.7/library/os.html#os.chmod

And it looks annoying bc you need to import stat

Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe we should just require the user to make sure their script is executable? Same for the prepare_assets.sh script

Copy link
Contributor Author

@fiendish fiendish May 22, 2020

Choose a reason for hiding this comment

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

Grumble!

At least prepare_assets is called from bash, so hopefully that'll be ok

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@fiendish
Copy link
Contributor Author

fiendish commented May 22, 2020

rebased and squashed after latest changes

@fiendish fiendish requested a review from znatty22 May 22, 2020 15:42
znatty22
znatty22 previously approved these changes May 24, 2020
Copy link
Collaborator

@znatty22 znatty22 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! Are you going to 🔖 it?!

@fiendish
Copy link
Contributor Author

fiendish commented Jun 1, 2020

🤔 hmm

I'm not sure that the symlink will work. But we can try!

@fiendish
Copy link
Contributor Author

fiendish commented Jun 1, 2020

Actually, I'll test that on a fake repo first.

@fiendish
Copy link
Contributor Author

fiendish commented Jun 1, 2020

Yeah. GH doesn't treat the symlink as a symlink, so I have to remove it. :(

I also sent GH a feature request to support symlinks in the workflows directory.

Copy link
Collaborator

@znatty22 znatty22 left a comment

Choose a reason for hiding this comment

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

I couldn't comment on the file itself, so I'll put the comment here. If you put a version variable in your __init__.py and import it in setup.py to set the version arg, it might be easier to write a pre_release.sh script that just updates __init__.py with the new version during the release process. Then when you release this, you won't have to do it manually.

@fiendish
Copy link
Contributor Author

fiendish commented Jun 1, 2020

I couldn't comment on the file itself, so I'll put the comment here. If you put a version variable in your __init__.py and import it in setup.py to set the version arg, it might be easier to write a pre_release.sh script that just updates __init__.py with the new version during the release process. Then when you release this, you won't have to do it manually.

What about this instead? It automatically pulls the package version from the git tag. d07ecec

@fiendish fiendish requested a review from znatty22 June 1, 2020 16:49
Copy link
Collaborator

@znatty22 znatty22 left a comment

Choose a reason for hiding this comment

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

👍

@fiendish fiendish merged commit 1c49d0a into master Jun 2, 2020
@fiendish fiendish deleted the release-maker branch June 2, 2020 15:44
fiendish added a commit that referenced this pull request Jun 3, 2020
## Release 1.0.0

### Summary

- Emojis: 👷 x1, 🎉 x1
- Categories: Ops x1, Additions x1

### New features and changes

- [#2](#2) - 👷 Lint PRs - [a5bd6cc](a5bd6cc) by [fiendish](https://github.com/fiendish)
- [#1](#1) - 🎉 New release maker - [1c49d0a](1c49d0a) by [fiendish](https://github.com/fiendish)
@fiendish fiendish mentioned this pull request Jun 3, 2020
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.

None yet

3 participants