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 Release workflow #2236

Merged
merged 1 commit into from
Dec 18, 2024
Merged

Conversation

radcortez
Copy link
Member

@radcortez radcortez commented Dec 5, 2024

To use the new release process... I'm unsure if this will work on the first try (probably not :) and will require some adjustments).

This project also releases a Gradle plugin. I did that on the preparation workflow before executing the central one. The files should be updated and tagged:
https://github.com/smallrye/smallrye-graphql/pull/2236/files#diff-ddf2bab74923dcd43f9f8d05e50bb1adc9ee5366a394d151d1f04a54a6079486

There is a separate workflow that executes to do the actual Gradle release after the central one and tag:
https://github.com/smallrye/smallrye-graphql/pull/2236/files#diff-8d232d862a776c1047b335f66544717bd08e86abac7de606a02ffa018563476f

This also seems to require graphviz that most likely we don't have available in the central repo:
https://github.com/smallrye/smallrye-graphql/pull/2236/files#diff-87db21a973eed4fef5f32b267aa60fcee5cbdf03c67fafdc2a9b553bb0b15f34L38-L39

@radcortez radcortez requested a review from a team as a code owner December 5, 2024 18:59
@radcortez radcortez requested a review from gastaldi December 5, 2024 19:05

- name: gradle release ${{inputs.version}}
run: |
mkdir -p ~/.gradle ; echo -e "gradle.publish.key=${{secrets.GRADLE_PUBLISH_KEY}}\ngradle.publish.secret=${{secrets.GRADLE_PUBLISH_SECRET}}" > ~/.gradle/gradle.properties
Copy link

Choose a reason for hiding this comment

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

Can't you set them as environment variables?

- name: Prepare Gradle
echo "version=${{steps.metadata.outputs.current-version}}" > tools/gradle-plugin/gradle.properties
git add tools/gradle-plugin/gradle.properties
git commit -m "Update Gradle plugin version"
Copy link

Choose a reason for hiding this comment

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

You need to push the changes somewhere, the prepare-release job below won't execute in the same context as this job

Copy link

Choose a reason for hiding this comment

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

I'd recommend having a hook to perform the changes you need in the prepare-release.yml, like in https://github.com/quarkiverse/.github/blob/main/.github/workflows/prepare-release.yml#L118-L124

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, we need to do a git push there.

Then, the job below would just check out the new commit.

Copy link

Choose a reason for hiding this comment

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

For git push to work, it depends on the ruleset configured in this repository too (I don't know if the current user is able to bypass the branch restrictions). In any case, I think it would be better if it used the GitHub App token, as in
https://github.com/smallrye/.github/blob/main/.github/workflows/prepare-release.yml#L35-L40

@radcortez
Copy link
Member Author

This also seems to require graphviz that most likely we don't have available in the central repo:
https://github.com/smallrye/smallrye-graphql/pull/2236/files#diff-87db21a973eed4fef5f32b267aa60fcee5cbdf03c67fafdc2a9b553bb0b15f34L38-L39

@gastaldi what about this?

@gastaldi
Copy link

gastaldi commented Dec 6, 2024

@radcortez is graphviz required in gradle publishPlugins or when running release:prepare? If the latter, it would be better to change the prepare-release.yml in the smallrye/.github repository to support hooks, as I mentioned in https://github.com/smallrye/smallrye-graphql/pull/2236/files#r1871984725

@gastaldi
Copy link

gastaldi commented Dec 6, 2024

@radcortez see smallrye/.github#5

@radcortez
Copy link
Member Author

@radcortez is graphviz required in gradle publishPlugins or when running release:prepare? If the latter, it would be better to change the prepare-release.yml in the smallrye/.github repository to support hooks, as I mentioned in https://github.com/smallrye/smallrye-graphql/pull/2236/files#r1871984725

I think it is required for javadocs:

smallrye-graphql/pom.xml

Lines 477 to 489 in 95641c9

<plugin>
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-javadoc-plugin</artifactId>
<version>3.5.0</version>
<configuration>
<doclet>nl.talsmasoftware.umldoclet.UMLDoclet</doclet>
<docletArtifact>
<groupId>nl.talsmasoftware</groupId>
<artifactId>umldoclet</artifactId>
<version>2.0.12</version>
</docletArtifact>
</configuration>
</plugin>

@jmartisk
Copy link
Member

I am not really sure what that is used for, and I think we may not need it at all - but maybe @phillip-kruger can correct me

@jmartisk
Copy link
Member

Hm, looking at these diagrams, they look a bit crazy, and TBH I don't know if useful. Maybe if it complicates the release too much, we could remove it

@phillip-kruger
Copy link
Member

Your call.

@radcortez
Copy link
Member Author

It shouldn't complicate the release, but we do have to add https://github.com/smallrye/smallrye-graphql/pull/2236/files#diff-87db21a973eed4fef5f32b267aa60fcee5cbdf03c67fafdc2a9b553bb0b15f34L38-L39 to the action running the release.

I don't see a big issue in it. What do you think @gastaldi?

@gastaldi
Copy link

@radcortez when smallrye/.github#5 is merged, you can add a .github/workflows/hooks/pre-prepare-release.sh file to this repository with the commands to install it

@radcortez
Copy link
Member Author

Hm, looking at these diagrams, they look a bit crazy, and TBH I don't know if useful. Maybe if it complicates the release too much, we could remove it

@gastaldi has a way to install the necessary components to support it. Is this something you want to keep so we can prepare the scripts for it?

@jmartisk
Copy link
Member

If it's not a big problem, then let's keep it, but I wouldn't mind dropping it either.

@radcortez
Copy link
Member Author

I've tried locally, and it seems to build fine without it. Let me try it without.

@radcortez radcortez merged commit 5c1ccb0 into smallrye:main Dec 18, 2024
4 checks passed
@github-actions github-actions bot added this to the 2.8.7 milestone Dec 18, 2024
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.

4 participants