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

create github actions #124

Closed
wants to merge 9 commits into from
Closed

create github actions #124

wants to merge 9 commits into from

Conversation

Richardson-e
Copy link

No description provided.

@@ -0,0 +1,10 @@
contact_links:
- name: Grails Core Discussions
url: https://github.com/grails/grails-core/discussions
Copy link

Choose a reason for hiding this comment

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

Grails Core Discussions record should be removed; we are not using grails-core/discussions

Copy link
Author

Choose a reason for hiding this comment

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

got it --addressed


runs-on: ubuntu-latest
env:
GIT_USER_NAME: puneetbehl
Copy link

Choose a reason for hiding this comment

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

This is fine for now; adding this comment as a reminder that we should consider getting a user/email here for the Grails account/group/project/whatever, rather than using a person account (in this case, Puneet's).

@Richardson-e Richardson-e requested a review from mattmoss February 8, 2024 18:16
@@ -0,0 +1,8 @@
contact_links:
about: Ask questions about Grails® framework on Github
Copy link

Choose a reason for hiding this comment

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

I think this line 'about' is leftover from removing discussions record; need to remove it as well.

@puneetbehl
Copy link
Contributor

@Richardson-e Please set-up a meeting tomorrow with me. I would like to understand a few things in this PR.

@puneetbehl
Copy link
Contributor

@Richardson-e Are you able to address the changes we discussed in the last Grails Engineering meeting?

@github-advanced-security
Copy link

This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation.

Copy link

@guillermocalvo guillermocalvo left a comment

Choose a reason for hiding this comment

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

@Emrichardsone @puneetbehl
FYI I just pushed some changes:

  • I modified the target branches so that actions can run on this PR before we actually merge it.
  • I had to restrict Java to version 8, because grails-quartz is still on Gradle 3, which does not support newer JDKs.
  • I also removed actions sdkman and groovy-joint-workflow because I believe @puneetbehl said we didn't need those here.

It might be a good idea to try and upgrade Gradle once we merge this PR.

arguments: -Psigning.secretKeyRingFile=${{ github.workspace }}/secring.gpg publishToSonatype closeAndReleaseSonatypeStagingRepository
- name: Run post-release
if: success()
uses: ./.github/actions/post-release
Copy link
Contributor

Choose a reason for hiding this comment

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

I think there is no need to use custom action here.

with:
name: grails-${{ steps.release_version.outputs.value }}.zip
path: build/distributions/grails-${{ steps.release_version.outputs.value }}.zip
- name: Upload artifacts to the Github release
Copy link
Contributor

@puneetbehl puneetbehl Feb 23, 2024

Choose a reason for hiding this comment

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

I believe this would break as there isn't any build/distribution/grails-.. file.

@@ -0,0 +1,45 @@
version: 2
Copy link
Contributor

Choose a reason for hiding this comment

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

This file seems to be configured incorrectly. Also, I think we do not need to set-up dependabot updates.

on:
push:
branches:
- '[1-9]+.[0-9]+.x'
Copy link
Contributor

Choose a reason for hiding this comment

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

I thinks it should be configured starting branch 3.0.x and above

Choose a reason for hiding this comment

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

@puneetbehl Please see my other comment about this.

- '[3-9]+.[3-9]+.x'
pull_request:
branches:
- '[1-9]+.[0-9]+.x'
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should be similar to push to branches.

Choose a reason for hiding this comment

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

If we set on.pull_request.branches to:

      - '[4-9]+.[0-9]+.x'
      - '[3-9]+.[3-9]+.x'

Then the action will not be triggered for this PR, because it targets branch 3.0.x. In fact, I don't see the point in having several of these regular expressions, instead of just one catch-all [1-9]+.[0-9]+.x (both for on.pull_request.branches and on.push.branches) 🤔 Is there any particular reason to do things that way @puneetbehl ?

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe the reason we have multiple regex to skip executing this workflow for older branches.

Choose a reason for hiding this comment

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

@puneetbehl But this workflow will not be executed against older branches unless we actually merge this PR into them. So I don't think we need to skip anything, really 🤔

In any case, if for some reason we wanted to avoid executing this workflow for versions below 3.0.x, we could do that with just one regex: [3-9]+.[0-9]+.x.

GRADLE_ENTERPRISE_BUILD_CACHE_NODE_USER: ${{ secrets.GRADLE_ENTERPRISE_BUILD_CACHE_NODE_USER }}
GRADLE_ENTERPRISE_BUILD_CACHE_NODE_KEY: ${{ secrets.GRADLE_ENTERPRISE_BUILD_CACHE_NODE_KEY }}
with:
arguments: build groovydoc
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not sure how the docs are generated but based on that information we may want to execute a different task to generate documentation

ARTIFACTORY_PASSWORD: ${{ secrets.ARTIFACTORY_PASSWORD }}
with:
arguments: -Dorg.gradle.internal.publish.checksums.insecure=true publish
invoke-third-party-workflows:
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think there are any third-part workflows.

uses: actions/upload-artifact@c7d193f32edcb7bfad88892161225aeda64e9392 # v4
with:
name: grails-${{ steps.release_version.outputs.value }}.zip
path: build/distributions/grails-${{ steps.release_version.outputs.value }}.zip
Copy link
Contributor

Choose a reason for hiding this comment

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

Please verify if build/distribution/grails- is the correct path prefix in this case.

id: upload_artifact
uses: Roang-zero1/github-upload-release-artifacts-action@master
with:
args: build/distributions/grails-${{ steps.release_version.outputs.value }}.zip
Copy link
Contributor

Choose a reason for hiding this comment

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

Please verify the artifact path.

echo "value={\"grails_version\":\"$RELEASE_VERSION\"}" >> $GITHUB_OUTPUT
env:
RELEASE_VERSION: ${{ needs.publish.outputs.release_version }}
- name: Invoke grails-doc release workflow
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this isn't required.

token: ${{ secrets.GITHUB_TOKEN }}
env:
SNAPSHOT_SUFFIX: -SNAPSHOT
docs:
Copy link
Contributor

Choose a reason for hiding this comment

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

We may need to rethink on what and how to publish docs. Please look into some other Grails projects for inspiration.

uses: Roang-zero1/github-upload-release-artifacts-action@v3
with:
created_tag: v${{ github.event.inputs.release }}
args: build/distributions/grails-${{ steps.release_version.outputs.release_version }}.zip
Copy link
Contributor

Choose a reason for hiding this comment

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

Please crosscheck the distribution path

Choose a reason for hiding this comment

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

I don't think grails-quartz creates any files in build/distributions/grails-*. The artifact should be something like build/libs/quartz-VERSION.jar.

@Emrichardsone Were these actions created using the ones in grails-core as templates? Because those do a lot of things that are not needed for basic plugins.

echo "value::{\"grails_version\":\"$RELEASE_VERSION\"}" >> $GITHUB_OUTPUT
env:
RELEASE_VERSION: ${{ steps.release_version.outputs.release_version }}
- name: Invoke grails-doc release workflow
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this step is invalid here

if: steps.assemble.outcome == 'success'
id: grails_doc
uses: benc-uk/workflow-dispatch@v1.2
with:
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this step is invalid here

@puneetbehl
Copy link
Contributor

@Richardson-e Please take a look at my comments. There might be some other similar places. Also, I would recommend going through each workflow config file one by one and crosscheck what it might do? does it make sense in this project? and Is there something else which might be missing?

uses: Roang-zero1/github-upload-release-artifacts-action@v3
with:
created_tag: v${{ github.event.inputs.release }}
args: build/distributions/grails-${{ steps.release_version.outputs.release_version }}.zip

Choose a reason for hiding this comment

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

I don't think grails-quartz creates any files in build/distributions/grails-*. The artifact should be something like build/libs/quartz-VERSION.jar.

@Emrichardsone Were these actions created using the ones in grails-core as templates? Because those do a lot of things that are not needed for basic plugins.

@Richardson-e
Copy link
Author

Richardson-e commented Feb 29, 2024

@guillermocalvo yes, I used grails-core as a template. Would it be better if I started fresh using this as a template?

@guillermocalvo
Copy link

@guillermocalvo yes, I used grails-core as a template. Would it be better if I started fresh using this as a template?

Yeah, it may be worth starting fresh 🤔 WDYT @puneetbehl?

@puneetbehl
Copy link
Contributor

As previously suggested, it is advisable to review each file and comprehend every step. While starting from scratch may not be necessary in this scenario, gaining this understanding will facilitate the creation of a new workflow if the need arises. I am not opposed to starting afresh or modifying what you have?

You can draw inspiration from project workflow configurations, such as those in grails-core, grails-gsp, grails-views, grails-data-mapping, grails-spring-security-oauth2, etc. If any step or workflow is unclear, please don't hesitate to reach out for clarification.

@CLAassistant
Copy link

CLAassistant commented Mar 26, 2024

CLA assistant check
All committers have signed the CLA.

@puneetbehl puneetbehl closed this Mar 29, 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.

6 participants