stevedlawrence commented on code in PR #17:
URL:
https://github.com/apache/daffodil-infrastructure/pull/17#discussion_r2368463893
##########
containers/check-release/README.md:
##########
@@ -36,8 +36,34 @@ To use the container image to check a release, run the
following:
Alternatively, if you would like to do the same checks but also check for
reproducibility, use the Release Candidate Container to build a release
-directory directory, then run the following:
+directory, then run the following:
podman run -it --rm \
- --volume <RELEASE_DIR>:/release
- daffodil-check-release "<DIST_URL>" "<MAVEN_URL>" /release
+ --volume <RELEASE_DIR>:/release \
+ daffodil-check-release "<DIST_URL>" "<MAVEN_URL>" /release
+
+To use the container image to check a local release, a few additional
+steps are required. First, build the release directory as described
+in the `build-release/README.md` file using rc0 as the release label.
Review Comment:
I think I would do the build release stuff last. So that the intrusctions
are first about how to setup and configure forks, what changes need to be made,
and now to trigger the action and download the release.zip.
That can be followed by something that just says if you want to validation
things with the check-release container, follow its steps with XYZ
modifications.
##########
actions/release-candidate/src/main.js:
##########
@@ -114,7 +114,12 @@ async function run() {
fs.appendFileSync(`${ sbt_dir }/plugins/build.sbt`,
'addSbtPlugin("com.github.sbt" % "sbt-pgp" % "2.1.2")\n');
fs.appendFileSync(`${ sbt_dir }/build.sbt`, `pgpSigningKey :=
Some("${ gpg_signing_key_id }")\n`);
- if (publish) {
+ // enable SBT for publishing SBOM
+ fs.appendFileSync(`${ sbt_dir }/plugins/build.sbt`,
'addSbtPlugin("com.github.sbt" %% "sbt-sbom" % "0.4.0")\n');
+ fs.appendFileSync(`${ sbt_dir }/build.sbt`, `bomFormat := "xml"\n`);
Review Comment:
This uses backticks around the `bomFormat := ...` parameter. But in .js I
think backticks should only be used for templates where you want to expand
variables, like in the first parameter. So the second parameter wants to either
use single quotes or double quotes, and since the actual string contains double
quotes single quotes is probably preferred. This also matches the above usage
for sbt pgp configuration.
##########
containers/check-release/README.md:
##########
@@ -36,8 +36,34 @@ To use the container image to check a release, run the
following:
Alternatively, if you would like to do the same checks but also check for
reproducibility, use the Release Candidate Container to build a release
-directory directory, then run the following:
+directory, then run the following:
podman run -it --rm \
- --volume <RELEASE_DIR>:/release
- daffodil-check-release "<DIST_URL>" "<MAVEN_URL>" /release
+ --volume <RELEASE_DIR>:/release \
+ daffodil-check-release "<DIST_URL>" "<MAVEN_URL>" /release
+
+To use the container image to check a local release, a few additional
+steps are required. First, build the release directory as described
+in the `build-release/README.md` file using rc0 as the release label.
+Then build the check-release container as described above, and push your
+changes to your fork. Then, update the `uses` action of the
+`ASF Release Candidate` step in the
+`.github/workflows/release-candidate.yml` file in the Daffodil repository.
+Push your changes to your fork on that repository as well. You will then need
+to add the secrets required by the `ASF Release Candidate` step to your
+Daffodil fork. The following secrets are required:
+DAFFODIL_GPG_SECRET_KEY, DAFFODIL_SVN_DEV_USERNAME, DAFFODIL_SVN_DEV_PASSWORD,
+NEXUS_STAGE_DEPLOYER_USER, NEXUS_STAGE_DEPLOYER_PW. The first
+can be any private key without a passphrase. The other
+secrets can be set to any non-empty value, as they will not be used.
+Now you can generate a local release, by going to the Actions tab of Daffodil
fork,
+and selecting the `Release Candidate` workflow. Click the `Run workflow`
button,
Review Comment:
Avoid "Release Candiate" since we don't know what the workflow will be
called. Daffodil projects use Release Candiate, but we should have some other
slightly more generic way to reference the GitHub Action that uses the
release-candidate action.
##########
containers/check-release/README.md:
##########
@@ -36,8 +36,34 @@ To use the container image to check a release, run the
following:
Alternatively, if you would like to do the same checks but also check for
reproducibility, use the Release Candidate Container to build a release
-directory directory, then run the following:
+directory, then run the following:
podman run -it --rm \
- --volume <RELEASE_DIR>:/release
- daffodil-check-release "<DIST_URL>" "<MAVEN_URL>" /release
+ --volume <RELEASE_DIR>:/release \
+ daffodil-check-release "<DIST_URL>" "<MAVEN_URL>" /release
+
+To use the container image to check a local release, a few additional
+steps are required. First, build the release directory as described
+in the `build-release/README.md` file using rc0 as the release label.
+Then build the check-release container as described above, and push your
+changes to your fork. Then, update the `uses` action of the
+`ASF Release Candidate` step in the
+`.github/workflows/release-candidate.yml` file in the Daffodil repository.
Review Comment:
We should avoid using "Daffodil" here, since these same steps work for SBT,
VSCode, or potentially other repos that want to use this--there actually isn't
anything Daffodil specific in the release candidate action so we should avoid
using daffodil and just mention different forks (e.g. the
daffodil-infrastructure fork and the action .yml file that `uses` the action.
##########
actions/release-candidate/src/main.js:
##########
@@ -114,7 +114,12 @@ async function run() {
fs.appendFileSync(`${ sbt_dir }/plugins/build.sbt`,
'addSbtPlugin("com.github.sbt" % "sbt-pgp" % "2.1.2")\n');
fs.appendFileSync(`${ sbt_dir }/build.sbt`, `pgpSigningKey :=
Some("${ gpg_signing_key_id }")\n`);
- if (publish) {
+ // enable SBT for publishing SBOM
+ fs.appendFileSync(`${ sbt_dir }/plugins/build.sbt`,
'addSbtPlugin("com.github.sbt" %% "sbt-sbom" % "0.4.0")\n');
+ fs.appendFileSync(`${ sbt_dir }/build.sbt`, `bomFormat := "xml"\n`);
+
+
+ if (publish) {
Review Comment:
This .js file uses tabs for indentation. Can you change your new code so
that it changes your spaces to tabs?
I have no preference in tabs vs spaces, and I'm fine if we want to switch to
spaces, but we should make this PR be consistent with the current style.
##########
containers/check-release/src/check-release.sh:
##########
@@ -80,30 +80,34 @@ RELEASE_DIR=release-download
DIST_DIR=$RELEASE_DIR/asf-dist
MAVEN_DIR=$RELEASE_DIR/maven-local
-printf "\n==== Downloading Release Files ====\n"
-# download dist/dev/ files
-mkdir -p $DIST_DIR
-pushd $DIST_DIR &>/dev/null
-download_dir $DIST_URL
-popd &>/dev/null
-
-# download maven repository, delete nexus generated files, and remove the
-# orgapachedaffodil-1234 dir since the build-release container does not have
-# this directory
-if [ -n "$MAVEN_URL" ]
+if [ ! -d "$RELEASE_DIR" ]
then
- mkdir -p $MAVEN_DIR
- pushd $MAVEN_DIR &>/dev/null
- download_dir $MAVEN_URL
- find . -type f \( -name 'archetype-catalog.xml' -o -name
'maven-metadata.xml*' \) -delete
- REPO_DIR=(*/)
- mv $REPO_DIR/* .
- rmdir $REPO_DIR
- popd &>/dev/null
-fi
+ printf "\n==== Downloading Release Files ====\n"
-printf "\n==== Download Complete ====\n"
+ # download dist/dev/ files
+ mkdir -p $DIST_DIR
+ pushd $DIST_DIR &>/dev/null
+ download_dir $DIST_URL
+ popd &>/dev/null
+
+ # download maven repository, delete nexus generated files, and remove the
+ # orgapachedaffodil-1234 dir since the build-release container does not have
+ # this directory
+ if [ -n "$MAVEN_URL" ]
+ then
+ mkdir -p $MAVEN_DIR
+ pushd $MAVEN_DIR &>/dev/null
+ download_dir $MAVEN_URL
+ find . -type f \( -name 'archetype-catalog.xml' -o -name
'maven-metadata.xml*' \) -delete
+ REPO_DIR=(*/)
+ mv $REPO_DIR/* .
+ rmdir $REPO_DIR
+ popd &>/dev/null
+ fi
+
+ printf "\n==== Download Complete ====\n"
+fi
Review Comment:
Can we add an else that says something like
> printf "\n==== Skipping Download, release-download/ directory already
exists ====\n"
That way if someone runs check release twice it will be clear that if they
needed to delete the directory if they want to redownload files?
##########
containers/check-release/src/check-release.sh:
##########
@@ -80,30 +80,34 @@ RELEASE_DIR=release-download
DIST_DIR=$RELEASE_DIR/asf-dist
MAVEN_DIR=$RELEASE_DIR/maven-local
-printf "\n==== Downloading Release Files ====\n"
-# download dist/dev/ files
-mkdir -p $DIST_DIR
-pushd $DIST_DIR &>/dev/null
-download_dir $DIST_URL
-popd &>/dev/null
-
-# download maven repository, delete nexus generated files, and remove the
-# orgapachedaffodil-1234 dir since the build-release container does not have
-# this directory
-if [ -n "$MAVEN_URL" ]
+if [ ! -d "$RELEASE_DIR" ]
then
- mkdir -p $MAVEN_DIR
- pushd $MAVEN_DIR &>/dev/null
- download_dir $MAVEN_URL
- find . -type f \( -name 'archetype-catalog.xml' -o -name
'maven-metadata.xml*' \) -delete
- REPO_DIR=(*/)
- mv $REPO_DIR/* .
- rmdir $REPO_DIR
- popd &>/dev/null
-fi
+ printf "\n==== Downloading Release Files ====\n"
Review Comment:
This file uses tabs instead of spaces. Like a above, I have no preference
either way, but this PR should be consistent. We can change it later if we want.
##########
containers/check-release/README.md:
##########
@@ -36,8 +36,34 @@ To use the container image to check a release, run the
following:
Alternatively, if you would like to do the same checks but also check for
reproducibility, use the Release Candidate Container to build a release
-directory directory, then run the following:
+directory, then run the following:
podman run -it --rm \
- --volume <RELEASE_DIR>:/release
- daffodil-check-release "<DIST_URL>" "<MAVEN_URL>" /release
+ --volume <RELEASE_DIR>:/release \
+ daffodil-check-release "<DIST_URL>" "<MAVEN_URL>" /release
+
+To use the container image to check a local release, a few additional
Review Comment:
> To use the container image to check a local release ...
I'm not sure "local release" is the best way to describe what this paragraph
is talking about. I also wonder if this wants to be part of a new "Testing"
section in the release-candidate action README, since most of these steps are
about configuring a fork to be able to run the action and checking those
results. It would also be more legible if the steps were in a numbered list. So
maybe something more like:
```
# Testing
Perform the following steps to test changes to the daffodil-infrastructure
repo on a GitHub fork:
1. Foo ...
1. Bar ...
1. Baz ...
```
--
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]
