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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

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

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