Copilot commented on code in PR #16252:
URL: https://github.com/apache/grails-core/pull/16252#discussion_r3882686478
##########
grails-forge/infrastructure/shared.yaml:
##########
@@ -285,7 +285,10 @@ Resources:
- Fn::Sub:
arn:${AWS::Partition}:elasticbeanstalk:${AWS::Region}:${AWS::AccountId}:environment/${ApplicationName}/${ApplicationName}-prev
- Fn::Sub:
arn:${AWS::Partition}:elasticbeanstalk:${AWS::Region}:${AWS::AccountId}:environment/${ApplicationName}/${ApplicationName}-prev-snapshot
- Fn::Sub:
arn:${AWS::Partition}:elasticbeanstalk:${AWS::Region}:${AWS::AccountId}:applicationversion/${ApplicationName}/*
- - Action: cloudformation:DescribeStacks
+ - Action:
+ - cloudformation:DescribeStacks
+ - cloudformation:DescribeStackResources
+ - cloudformation:GetTemplate
Effect: Allow
Resource: '*'
Review Comment:
Adding `cloudformation:DescribeStackResources` and
`cloudformation:GetTemplate` with `Resource: '*'` broadens permissions
significantly. If possible, scope `Resource` to the specific shared stack
ARN(s) (or apply a condition on `cloudformation:StackName`) to keep the policy
least-privilege while still enabling EB’s `UpdateEnvironment` flow.
##########
grails-forge/grails-forge-web-netty/build.gradle:
##########
@@ -124,20 +123,23 @@ tasks.named('dockerBuild') {
images = [findProperty('dockerImageName') ?: 'grailsforge']
}
-TaskProvider<ShadowJar> shadowJarTask = tasks.named('shadowJar', ShadowJar)
-shadowJarTask.configure {
- reproducibleFileOrder = true
- preserveFileTimestamps = false
+['com.gradleup.shadow', 'com.github.johnrengelman.shadow'].each { pluginId ->
+ pluginManager.withPlugin(pluginId) {
+ tasks.named('shadowJar', ShadowJar).configure {
+ reproducibleFileOrder = true
+ preserveFileTimestamps = false
+ }
+ }
}
TaskProvider<Zip> awsElasticBeanstalk = tasks.register('awsElasticBeanstalk',
Zip) {
- dependsOn(shadowJarTask)
+ dependsOn('shadowJar')
Review Comment:
The ShadowJar configuration is conditional on a shadow plugin being applied,
but `awsElasticBeanstalk` unconditionally depends on and resolves `shadowJar`.
If neither plugin is applied (or Micronaut changes its defaults),
`dependsOn('shadowJar')` / `tasks.named('shadowJar')` will fail at
configuration/execution. Consider explicitly applying a shadow plugin (without
a version, if it’s already on the buildscript classpath) or making
`awsElasticBeanstalk` registration/config conditional on the plugin being
present.
##########
grails-forge/grails-forge-web-netty/build.gradle:
##########
@@ -124,20 +123,23 @@ tasks.named('dockerBuild') {
images = [findProperty('dockerImageName') ?: 'grailsforge']
}
-TaskProvider<ShadowJar> shadowJarTask = tasks.named('shadowJar', ShadowJar)
-shadowJarTask.configure {
- reproducibleFileOrder = true
- preserveFileTimestamps = false
+['com.gradleup.shadow', 'com.github.johnrengelman.shadow'].each { pluginId ->
+ pluginManager.withPlugin(pluginId) {
+ tasks.named('shadowJar', ShadowJar).configure {
+ reproducibleFileOrder = true
+ preserveFileTimestamps = false
+ }
+ }
}
TaskProvider<Zip> awsElasticBeanstalk = tasks.register('awsElasticBeanstalk',
Zip) {
- dependsOn(shadowJarTask)
+ dependsOn('shadowJar')
archiveFileName.set('grails-forge-web-netty-aws.zip')
destinationDirectory.set(layout.buildDirectory.dir('distributions'))
reproducibleFileOrder = true
preserveFileTimestamps = false
- from(shadowJarTask.flatMap { it.archiveFile }) {
+ from({ tasks.named('shadowJar').get().archiveFile.get() }) {
rename { 'app.jar' }
}
Review Comment:
The ShadowJar configuration is conditional on a shadow plugin being applied,
but `awsElasticBeanstalk` unconditionally depends on and resolves `shadowJar`.
If neither plugin is applied (or Micronaut changes its defaults),
`dependsOn('shadowJar')` / `tasks.named('shadowJar')` will fail at
configuration/execution. Consider explicitly applying a shadow plugin (without
a version, if it’s already on the buildscript classpath) or making
`awsElasticBeanstalk` registration/config conditional on the plugin being
present.
##########
grails-forge/grails-forge-web-netty/build.gradle:
##########
@@ -124,20 +123,23 @@ tasks.named('dockerBuild') {
images = [findProperty('dockerImageName') ?: 'grailsforge']
}
-TaskProvider<ShadowJar> shadowJarTask = tasks.named('shadowJar', ShadowJar)
-shadowJarTask.configure {
- reproducibleFileOrder = true
- preserveFileTimestamps = false
+['com.gradleup.shadow', 'com.github.johnrengelman.shadow'].each { pluginId ->
+ pluginManager.withPlugin(pluginId) {
+ tasks.named('shadowJar', ShadowJar).configure {
+ reproducibleFileOrder = true
+ preserveFileTimestamps = false
+ }
+ }
}
TaskProvider<Zip> awsElasticBeanstalk = tasks.register('awsElasticBeanstalk',
Zip) {
- dependsOn(shadowJarTask)
+ dependsOn('shadowJar')
archiveFileName.set('grails-forge-web-netty-aws.zip')
destinationDirectory.set(layout.buildDirectory.dir('distributions'))
reproducibleFileOrder = true
preserveFileTimestamps = false
- from(shadowJarTask.flatMap { it.archiveFile }) {
+ from({ tasks.named('shadowJar').get().archiveFile.get() }) {
Review Comment:
This resolves the `shadowJar` task and its `archiveFile` eagerly via
`.get()`, which defeats Gradle’s lazy configuration and can hurt configuration
cache compatibility. Prefer wiring `from(...)` to a Provider (e.g.,
`tasks.named('shadowJar').flatMap { ... }`) so the archive file is realized
lazily.
##########
.github/workflows/forge-deploy-aws.yml:
##########
@@ -13,13 +13,11 @@
# See the License for the specific language governing permissions and
# limitations under the License.
#
-# GitHub registers workflow_dispatch from the default branch. Dispatch from
that
-# branch and set source_ref to the maintenance line or tag to build. The job
-# reads javaVersion from gradle.properties. OIDC trusts refs/heads/*.x and
-# refs/tags/v* in apache/grails-core, so new version branches need no template
edit.
+# Dispatch from the maintenance branch that should be built (Use workflow
from).
+# Choose the slot. Region, stack name, and JDK come from that branch.
Review Comment:
The header comment says region and stack name come from the selected branch,
but the workflow now hard-codes `us-east-1` and `grails-forge-shared`. Please
update the comment to match the actual behavior (only the JDK is coming from
the branch via `gradle.properties`).
##########
.github/workflows/forge-deploy-aws.yml:
##########
@@ -65,34 +45,17 @@ jobs:
deploy:
name: "Deploy ${{ inputs.slot }} to AWS Elastic Beanstalk"
runs-on: ubuntu-24.04
- timeout-minutes: 75
+ timeout-minutes: 40
env:
- AWS_REGION: ${{ inputs.aws_region }}
- AWS_DEFAULT_REGION: ${{ inputs.aws_region }}
+ AWS_REGION: us-east-1
+ AWS_DEFAULT_REGION: us-east-1
AWS_PAGER: ""
DEPLOY_ROLE_ARN: ${{ vars.AWS_FORGE_DEPLOY_ROLE_ARN }}
- RELEASE: ${{ inputs.release }}
- SHARED_STACK: ${{ inputs.shared_stack }}
+ SHARED_STACK: grails-forge-shared
Review Comment:
The header comment says region and stack name come from the selected branch,
but the workflow now hard-codes `us-east-1` and `grails-forge-shared`. Please
update the comment to match the actual behavior (only the JDK is coming from
the branch via `gradle.properties`).
##########
.github/workflows/forge-deploy-aws.yml:
##########
@@ -65,34 +45,17 @@ jobs:
deploy:
name: "Deploy ${{ inputs.slot }} to AWS Elastic Beanstalk"
runs-on: ubuntu-24.04
- timeout-minutes: 75
+ timeout-minutes: 40
Review Comment:
The workflow timeout was reduced to 40 minutes, but the script can
legitimately spend up to ~20 minutes waiting for application version processing
plus up to ~15 minutes waiting for the environment to become Ready/Green
(excluding packaging/upload/update overhead). This leaves little buffer and
risks intermittent timeout failures; consider increasing the timeout or
tightening the waits if that’s the goal.
##########
.github/workflows/forge-deploy-aws.yml:
##########
@@ -276,14 +183,8 @@ jobs:
local exit_code=$?
trap - ERR
if [[ "${update_started}" == true && "${rollback_available}" ==
true ]]; then
- echo "Deployment failed. Rolling back ${ENVIRONMENT_NAME} to its
previous application version."
- if aws elasticbeanstalk update-environment --environment-name
"${ENVIRONMENT_NAME}" --version-label "${previous_version}" >/dev/null &&
wait_for_ready_green 'Rollback' "${previous_version}"; then
- echo "Rollback completed."
- else
- echo "Rollback did not reach Status=Ready and Health=Green.
Manual intervention is required." >&2
- fi
- elif [[ "${update_started}" == true ]]; then
- echo "Deployment failed, but no prior application version is
available for rollback." >&2
+ echo "Deployment failed. Rolling back ${ENVIRONMENT_NAME}."
+ aws elasticbeanstalk update-environment --environment-name
"${ENVIRONMENT_NAME}" --version-label "${previous_version}" >/dev/null || true
Review Comment:
Rollback now ignores errors (`|| true`) and does not wait for the
environment to return to a stable Ready/Green state, which can leave the
environment mid-update and make follow-on deploys/diagnosis harder. Consider
reintroducing a wait (and surfacing rollback failure) so the workflow ends in a
known state when rollback is attempted.
##########
.github/workflows/forge-deploy-aws.yml:
##########
@@ -123,85 +86,42 @@ jobs:
run: |
set -euo pipefail
case "${SLOT}" in
- latest)
- hostname='latest.grails.org'
- ;;
- snapshot)
- hostname='snapshot.grails.org'
- ;;
- next)
- hostname='next.grails.org'
- ;;
- prev)
- hostname='prev.grails.org'
- ;;
- prev-snapshot)
- hostname='prev-snapshot.grails.org'
- ;;
+ latest) hostname='latest.grails.org' ;;
+ snapshot) hostname='snapshot.grails.org' ;;
+ next) hostname='next.grails.org' ;;
+ prev) hostname='prev.grails.org' ;;
+ prev-snapshot) hostname='prev-snapshot.grails.org' ;;
*)
echo "Unsupported deployment slot" >&2
exit 1
;;
esac
-
- sanitized_release="$(printf '%s' "${RELEASE}" | tr '[:upper:]'
'[:lower:]' | tr -cs 'a-z0-9._-' '-')"
- sanitized_release="${sanitized_release#-}"
- sanitized_release="${sanitized_release%-}"
- label_prefix="${SLOT}"
- if [[ -n "${sanitized_release}" ]]; then
- label_prefix="${label_prefix}-${sanitized_release}"
- fi
source_sha="$(git rev-parse HEAD)"
-
label_suffix="-${source_sha:0:12}-${GITHUB_RUN_ID}-${GITHUB_RUN_ATTEMPT}"
- label_prefix="${label_prefix:0:$((100 - ${#label_suffix}))}"
- version_label="${label_prefix%-}${label_suffix}"
-
+
version_label="${SLOT}-${source_sha:0:12}-${GITHUB_RUN_ID}-${GITHUB_RUN_ATTEMPT}"
printf 'hostname=%s\n' "${hostname}" >> "${GITHUB_OUTPUT}"
printf 'version-label=%s\n' "${version_label}" >> "${GITHUB_OUTPUT}"
printf 's3-key=artifacts/%s/%s.zip\n' "${SLOT}" "${version_label}"
>> "${GITHUB_OUTPUT}"
- - name: "Test and package Forge web application"
+ - name: "Package Forge for Elastic Beanstalk"
working-directory: grails-forge
shell: bash
- run: >
- ./gradlew
- grails-forge-api:test
- grails-forge-web-netty:test
- grails-forge-web-netty:awsElasticBeanstalk
- env:
- TEST_BUILD_REPRODUCIBLE: "true"
-
- - name: "Verify Elastic Beanstalk package"
- shell: bash
- run: |
- set -euo pipefail
-
artifact='grails-forge/grails-forge-web-netty/build/distributions/grails-forge-web-netty-aws.zip'
- test -s "${artifact}"
-
- - name: "Verify AWS OIDC role configuration"
- shell: bash
- run: |
- set -euo pipefail
- if [[ -z "${DEPLOY_ROLE_ARN}" ]]; then
- echo "AWS_FORGE_DEPLOY_ROLE_ARN must be configured as a repository
variable." >&2
- exit 1
- fi
- if [[ ! "${DEPLOY_ROLE_ARN}" =~ ^arn:aws:iam::[0-9]{12}:role/.+$ ]];
then
- echo "AWS_FORGE_DEPLOY_ROLE_ARN must be an IAM role ARN." >&2
- exit 1
- fi
+ run: ./gradlew grails-forge-web-netty:awsElasticBeanstalk
- name: "Configure AWS credentials through OIDC"
uses:
aws-actions/configure-aws-credentials@e7f100cf4c008499ea8adda475de1042d6975c7b
# v6.2.0
with:
role-to-assume: ${{ env.DEPLOY_ROLE_ARN }}
- aws-region: ${{ inputs.aws_region }}
+ aws-region: us-east-1
- name: "Discover shared deployment resources"
id: shared
shell: bash
run: |
set -euo pipefail
+ if [[ -z "${DEPLOY_ROLE_ARN}" ]]; then
+ echo "AWS_FORGE_DEPLOY_ROLE_ARN must be configured as a repository
variable." >&2
+ exit 1
+ fi
Review Comment:
The `DEPLOY_ROLE_ARN` validation now happens after
`configure-aws-credentials` uses it. If the variable is missing/misconfigured,
the action will fail first (often with a less direct error). Move this
validation to a step before `configure-aws-credentials` for clearer failures.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
