Copilot commented on code in PR #16251:
URL: https://github.com/apache/grails-core/pull/16251#discussion_r3882685111
##########
.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:
This validation runs after the workflow already uses `DEPLOY_ROLE_ARN` in
`aws-actions/configure-aws-credentials`. If the variable is missing/empty, the
action will fail first with a less actionable error. Move this validation into
a dedicated step *before* the `configure-aws-credentials` step (or at least
before it’s referenced) so failures are deterministic and the error message is
surfaced.
##########
.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 comment says region and stack name come from the chosen branch, but the
workflow hard-codes `us-east-1` and `grails-forge-shared`. Update the comment
to reflect the actual behavior (only JDK comes from branch; region/stack are
fixed) to prevent confusion for operators.
##########
grails-forge/grails-forge-web-netty/build.gradle:
##########
@@ -73,20 +73,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:
This wiring forces task lookup/realization via `get()` and relies on the
string task name `shadowJar` existing. It’s more robust to use a Provider-based
dependency/input (e.g., `tasks.named('shadowJar')` as a provider and pass its
`archiveFile` provider directly) so Gradle can avoid early realization and fail
earlier with clearer errors if the Shadow task is not present.
##########
grails-forge/grails-forge-web-netty/build.gradle:
##########
@@ -73,20 +73,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:
This wiring forces task lookup/realization via `get()` and relies on the
string task name `shadowJar` existing. It’s more robust to use a Provider-based
dependency/input (e.g., `tasks.named('shadowJar')` as a provider and pass its
`archiveFile` provider directly) so Gradle can avoid early realization and fail
earlier with clearer errors if the Shadow task is not present.
##########
grails-forge/grails-forge-web-netty/build.gradle:
##########
@@ -73,20 +73,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
+ }
+ }
Review Comment:
If both Shadow plugins are ever applied (accidentally or via transitive
conventions), this block will configure `shadowJar` twice. Consider guarding so
the configuration runs only once (e.g., by tracking a flag) to avoid surprising
behavior and make troubleshooting easier.
##########
.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:
The rollback path now ignores failures (`|| true`) and no longer waits for
the environment to return to `Ready/Green`. This can leave the environment
mid-rollback (or rollback-failed) while the workflow exits, making incidents
harder to detect and leaving the system in an unknown state. Consider restoring
a wait/check for rollback completion (and surface rollback failures) even if
the main deployment fails.
##########
grails-forge/infrastructure/shared.yaml:
##########
@@ -275,7 +275,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:
The added CloudFormation permissions are granted on `Resource: '*'`. If
these actions can be scoped (e.g., to the specific shared stack ARN or via
conditions like `cloudformation:StackName`), narrowing the resource scope would
reduce blast radius for the deploy role while still enabling Elastic Beanstalk
updates.
--
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]
