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

Automatically publish releases on CI #1510

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from
Draft

Conversation

Egorand
Copy link
Collaborator

@Egorand Egorand commented Apr 6, 2023

No description provided.

@Egorand Egorand marked this pull request as draft April 6, 2023 16:38
@Egorand Egorand requested a review from swankjesse April 6, 2023 16:39
Comment on lines +7 to +8
tags:
- '[0-9]+.[0-9]+*'
Copy link
Collaborator

Choose a reason for hiding this comment

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

Our tags have three numbers, not two. Also the dot needs escaped since it matches any character, not just a dot. However, I would recommend just removing this completely and instead just use a full wildcard ** like we do in other projects: https://github.com/cashapp/molecule/blob/trunk/.github/workflows/release.yaml#L3-L6

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is based on the discussion @swankjesse & I had - we wanted to have basic tag format validation, that would accept valid versions, even ones we don't normally use (e.g., "1.0-SQUARE"), and reject obviously incorrect ones (e.g., "hello").

@@ -1,7 +1,6 @@
org.gradle.jvmargs='-Dfile.encoding=UTF-8'

GROUP=com.squareup
VERSION_NAME=1.14.0-SNAPSHOT
Copy link
Collaborator

Choose a reason for hiding this comment

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

Doesn't this break the ability to do Maven local deploys for testing?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It does, yep. This likely breaks more things, and will generally be non-ergonomic for local development.

Comment on lines +30 to +32
- name: Set release version
if: startsWith(github.ref, 'refs/tags/')
run: echo "VERSION_NAME=${GITHUB_REF#refs/tags/}" >> gradle.properties
Copy link
Collaborator

Choose a reason for hiding this comment

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

We had a setup like this in RxAndroid that was mostly forced on us from the RxJava Travis CI infrastructure. Personally, I was glad to see it go away when we moved to GitHub Actions and I greatly prefer the explicit version. In other projects I'm editing the README and CHANGELOG when I release to indicate the next version so I'm not sure whether there's a ton of value in automating only one location.

If you want to go down the automating route I would like to see a workflow that's manually dispatched where the version is in the input. It would automatically update the README, migrate the CHANGELOG entries from an "Unreleased" section to a versioned section, and do the gradle.properties update/tag/update dance.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That makes sense.

Taking a step back, the main quality of life improvements I'd love to get from this change is not having to build the project locally (and risking using a wrong JDK or incorrect environment setup in general) for release, and not having to host signing keys on my machine - these can be achieved even without these extra workflow improvements. Given that this setup likely needs much more work to be pleasant to use, I feel like sticking with the basic requirements, and leaving the rest for later. Let me know if you have thoughts!

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don’t like having to do 2 commits per release. Changing the property for the current release and again for the next snapshot is a bit clumsy.

I also don’t like how the tag and the property duplicate information.

I’d like to discuss our options on a VC, maybe next week?

@Egorand Egorand changed the base branch from master to main July 5, 2023 09:23
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.

3 participants