stevedlawrence commented on code in PR #20:
URL:
https://github.com/apache/daffodil-infrastructure/pull/20#discussion_r2519187905
##########
actions/release-candidate/dist/main/index.js:
##########
@@ -31855,46 +31855,59 @@ async function run() {
const tlp_dir = core.getInput("tlp_dir", { required: true });
const project_id = core.getInput("project_id", { required: true
});
const project_dir = core.getInput("project_dir");
- const gpg_signing_key = core.getInput("gpg_signing_key", {
required: true });
- const svn_username = core.getInput("svn_username", { required:
true });
- const svn_password = core.getInput("svn_password", { required:
true });
- const nexus_username = core.getInput("nexus_username", {
required: true });
- const nexus_password = core.getInput("nexus_password", {
required: true });
- let publish = core.getBooleanInput("publish");
-
- // import signing key into gpg and get it's key id
- let gpg_import_stdout = ""
- await exec("gpg", ["--batch", "--import", "--import-options",
"import-show"], {
- input: Buffer.from(gpg_signing_key),
- listeners: {
- stdout: (data) => { gpg_import_stdout +=
data.toString(); }
- }
- });
- const gpg_signing_key_id =
gpg_import_stdout.match("[0-9A-Z]{40}")[0];
- console.info("Using gpgp key id: " + gpg_signing_key_id);
+ const publish = core.getBooleanInput("publish");
+
+ // get the actual project version, this requires a 'VERSION'
file at
+ // the root of the repository
+ const project_version =
fs.readFileSync("VERSION").toString().trim();
+ const is_snapshot = project_version.includes("-SNAPSHOT");
+ const is_apache = process.env.GITHUB_REPOSITORY_OWNER ==
"apache";
+ const gitTagPrefix = "refs/tags/";
+ const is_tagged = github.context.eventName == "push" &&
github.context.ref.startsWith(gitTagPrefix);
+ const do_publish =
+ // Note that publishing could be disabled if the
publish input was explicitly set
Review Comment:
I find these split comments harder to read, and now that the conditions have
readable names I'm not sure they add much value. WHat are your thoughts on
doing something like
```ts
// The publish setting must be explicitly set to true in order actualy
publish artifacts. Note that
// this required but not sufficient--even if this setting is true, a number
of other factors (usually
// related to testing) can disable publishing (e.g. non-tagged, snapshot
build, non-ASF repository)
const do_publish = publish && is_tagged && !is_snapshot && !is_apache
```
##########
actions/release-candidate/dist/main/index.js:
##########
@@ -31855,46 +31855,59 @@ async function run() {
const tlp_dir = core.getInput("tlp_dir", { required: true });
const project_id = core.getInput("project_id", { required: true
});
const project_dir = core.getInput("project_dir");
- const gpg_signing_key = core.getInput("gpg_signing_key", {
required: true });
- const svn_username = core.getInput("svn_username", { required:
true });
- const svn_password = core.getInput("svn_password", { required:
true });
- const nexus_username = core.getInput("nexus_username", {
required: true });
- const nexus_password = core.getInput("nexus_password", {
required: true });
- let publish = core.getBooleanInput("publish");
-
- // import signing key into gpg and get it's key id
- let gpg_import_stdout = ""
- await exec("gpg", ["--batch", "--import", "--import-options",
"import-show"], {
- input: Buffer.from(gpg_signing_key),
- listeners: {
- stdout: (data) => { gpg_import_stdout +=
data.toString(); }
- }
- });
- const gpg_signing_key_id =
gpg_import_stdout.match("[0-9A-Z]{40}")[0];
- console.info("Using gpgp key id: " + gpg_signing_key_id);
+ const publish = core.getBooleanInput("publish");
+
+ // get the actual project version, this requires a 'VERSION'
file at
+ // the root of the repository
+ const project_version =
fs.readFileSync("VERSION").toString().trim();
+ const is_snapshot = project_version.includes("-SNAPSHOT");
+ const is_apache = process.env.GITHUB_REPOSITORY_OWNER ==
"apache";
+ const gitTagPrefix = "refs/tags/";
+ const is_tagged = github.context.eventName == "push" &&
github.context.ref.startsWith(gitTagPrefix);
+ const do_publish =
+ // Note that publishing could be disabled if the
publish input was explicitly set
+ // to false
+ publish
+ // require a pushed tag to enable publishing
+ && is_tagged
+ // require non-snapshot ASF builds to enable publishing
+ && (!is_snapshot && is_apache)
+ const gpg_signing_key = core.getInput("gpg_signing_key",
{required: do_publish});
+
+ let gpg_signing_key_id = "";
Review Comment:
Suggest we move this to down where gpg_signing_key_id is set, I think it can
be a const now.
##########
actions/release-candidate/dist/main/index.js:
##########
@@ -31908,27 +31921,38 @@ async function run() {
// triggered from a tag, so we fetch it manually so we
can verify its tag
await exec("git", ["fetch", "origin", "--deepen=1",
`+${ github.context.ref }:${ github.context.ref }`]);
- // make sure the tag is signed by a committer in the
KEYS file, this
- // command fails if the tag does not verify.
- await exec("git", ["tag", "--verify", release_version]);
+ if (do_publish) {
+ // if publishing, tags must be signed with a
committers key, download and import committer
+ // keys for verification
+ let committer_keys = "";
+ await exec("curl",
[`https://downloads.apache.org/${tlp_dir}/KEYS`], {
+ silent: true,
+ listeners: {
+ stdout: (data) => {
+ committer_keys +=
data.toString();
+ }
+ }
+ });
+ await exec("gpg", ["--batch", "--import"], {
+ input: Buffer.from(committer_keys)
+ });
+
+ // make sure the tag is signed by a committer
in the KEYS file, this
+ // command fails if the tag does not verify.
+ await exec("git", ["tag", "--verify",
release_version]);
+ }
Review Comment:
It might be a bit safer to move the `git tag --verify` line into the `if
(do_publish)` block where we configure keys and passwords.
My thinking is that in the future we could accidentally add a bug where
`is_tagged` is false but `do_publish` ends up being true. In that case this
`git tag --verify` won't get triggered and we won't verify tags. But if we put
this in the other do_publish block then we will always try to verify tags if we
are publishing things, and it should fail if there isn't a tag to verify.
Also, it might nice to move the curl and gpg import outside of the block.
That way this is_tagged block is purely about figuring out the name of the
release, and the stuff about gpg keys and verifing tags elsewhere? Maybe the
curl/imoprt goes after the gpg signing key import/generation so all the gpg
related initialization is in one spot?
##########
actions/release-candidate/README.md:
##########
Review Comment:
It might be worth adding a not in here that the public key needed to verify
the artifacts is included in the zip.
##########
actions/release-candidate/dist/main/index.js:
##########
@@ -31855,46 +31855,59 @@ async function run() {
const tlp_dir = core.getInput("tlp_dir", { required: true });
const project_id = core.getInput("project_id", { required: true
});
const project_dir = core.getInput("project_dir");
- const gpg_signing_key = core.getInput("gpg_signing_key", {
required: true });
- const svn_username = core.getInput("svn_username", { required:
true });
- const svn_password = core.getInput("svn_password", { required:
true });
- const nexus_username = core.getInput("nexus_username", {
required: true });
- const nexus_password = core.getInput("nexus_password", {
required: true });
- let publish = core.getBooleanInput("publish");
-
- // import signing key into gpg and get it's key id
- let gpg_import_stdout = ""
- await exec("gpg", ["--batch", "--import", "--import-options",
"import-show"], {
- input: Buffer.from(gpg_signing_key),
- listeners: {
- stdout: (data) => { gpg_import_stdout +=
data.toString(); }
- }
- });
- const gpg_signing_key_id =
gpg_import_stdout.match("[0-9A-Z]{40}")[0];
- console.info("Using gpgp key id: " + gpg_signing_key_id);
+ const publish = core.getBooleanInput("publish");
+
+ // get the actual project version, this requires a 'VERSION'
file at
+ // the root of the repository
+ const project_version =
fs.readFileSync("VERSION").toString().trim();
+ const is_snapshot = project_version.includes("-SNAPSHOT");
+ const is_apache = process.env.GITHUB_REPOSITORY_OWNER ==
"apache";
+ const gitTagPrefix = "refs/tags/";
+ const is_tagged = github.context.eventName == "push" &&
github.context.ref.startsWith(gitTagPrefix);
+ const do_publish =
+ // Note that publishing could be disabled if the
publish input was explicitly set
+ // to false
+ publish
+ // require a pushed tag to enable publishing
+ && is_tagged
+ // require non-snapshot ASF builds to enable publishing
+ && (!is_snapshot && is_apache)
+ const gpg_signing_key = core.getInput("gpg_signing_key",
{required: do_publish});
+
+ let gpg_signing_key_id = "";
+ if (gpg_signing_key.trim() === "") {
+ // Generate keypair (non-interactive)
+ await exec("gpg", ["--batch", "--yes", "--passphrase",
'', "--quick-generate-key", process.env.USER ], {
Review Comment:
Instead of `process.env.USER`, I'd suggest we use a name and email that
makes it very clear this is a key only to be used for temporary daffodil
testing. Maybe something like `"Apache Daffodil Test Release
<[email protected]>"`?
It might also be a good idea to set a one day expiration date for the key
just to make sure it's not really usable for anything except short term
testing, which is the purpose of this key. I think you can do this by adding
`1d` as the last argument.
##########
actions/release-candidate/dist/main/index.js:
##########
@@ -31855,46 +31855,59 @@ async function run() {
const tlp_dir = core.getInput("tlp_dir", { required: true });
const project_id = core.getInput("project_id", { required: true
});
const project_dir = core.getInput("project_dir");
- const gpg_signing_key = core.getInput("gpg_signing_key", {
required: true });
- const svn_username = core.getInput("svn_username", { required:
true });
- const svn_password = core.getInput("svn_password", { required:
true });
- const nexus_username = core.getInput("nexus_username", {
required: true });
- const nexus_password = core.getInput("nexus_password", {
required: true });
- let publish = core.getBooleanInput("publish");
-
- // import signing key into gpg and get it's key id
- let gpg_import_stdout = ""
- await exec("gpg", ["--batch", "--import", "--import-options",
"import-show"], {
- input: Buffer.from(gpg_signing_key),
- listeners: {
- stdout: (data) => { gpg_import_stdout +=
data.toString(); }
- }
- });
- const gpg_signing_key_id =
gpg_import_stdout.match("[0-9A-Z]{40}")[0];
- console.info("Using gpgp key id: " + gpg_signing_key_id);
+ const publish = core.getBooleanInput("publish");
+
+ // get the actual project version, this requires a 'VERSION'
file at
+ // the root of the repository
+ const project_version =
fs.readFileSync("VERSION").toString().trim();
+ const is_snapshot = project_version.includes("-SNAPSHOT");
+ const is_apache = process.env.GITHUB_REPOSITORY_OWNER ==
"apache";
+ const gitTagPrefix = "refs/tags/";
+ const is_tagged = github.context.eventName == "push" &&
github.context.ref.startsWith(gitTagPrefix);
+ const do_publish =
+ // Note that publishing could be disabled if the
publish input was explicitly set
+ // to false
+ publish
+ // require a pushed tag to enable publishing
+ && is_tagged
+ // require non-snapshot ASF builds to enable publishing
+ && (!is_snapshot && is_apache)
+ const gpg_signing_key = core.getInput("gpg_signing_key",
{required: do_publish});
Review Comment:
It might be worth adding a comment here, something like
```ts
// we only require the gpg_singing_key if we are actually going to publish
artifacts. If it is
// not required and not provided, we generate a temporary key so the
workflow can still
// sign artifacts
```
##########
actions/release-candidate/dist/main/index.js:
##########
@@ -31908,27 +31921,38 @@ async function run() {
// triggered from a tag, so we fetch it manually so we
can verify its tag
await exec("git", ["fetch", "origin", "--deepen=1",
`+${ github.context.ref }:${ github.context.ref }`]);
- // make sure the tag is signed by a committer in the
KEYS file, this
- // command fails if the tag does not verify.
- await exec("git", ["tag", "--verify", release_version]);
+ if (do_publish) {
+ // if publishing, tags must be signed with a
committers key, download and import committer
+ // keys for verification
+ let committer_keys = "";
+ await exec("curl",
[`https://downloads.apache.org/${tlp_dir}/KEYS`], {
+ silent: true,
+ listeners: {
+ stdout: (data) => {
+ committer_keys +=
data.toString();
+ }
+ }
+ });
+ await exec("gpg", ["--batch", "--import"], {
+ input: Buffer.from(committer_keys)
+ });
+
+ // make sure the tag is signed by a committer
in the KEYS file, this
+ // command fails if the tag does not verify.
+ await exec("git", ["tag", "--verify",
release_version]);
+ }
} else {
- // this was not triggered by a tag, maybe is was
manually triggered via
- // workflow_dispatch or a normal commit. We should only
publish from tags,
- // so we disable publishing. We also set the
release_version so that it has the
+ // this was not triggered by a tag, maybe it was
manually triggered via
+ // workflow_dispatch or a normal commit. We also set
the release_version so that it has the
// same format as a tag (e.g. v1.2.3-rc1)
core.warning("Action not triggered from tag, publishing
disabled");
release_version = `v${ project_version }-rc0`;
- publish = false;
}
- const is_snapshot = project_version.includes("-SNAPSHOT");
-
- // disable publishing for snapshot builds or non-ASF builds.
Note that
- // publishing could still be disabled if the publish input was
explicitly set
- // to false
- if (publish && (is_snapshot ||
process.env.GITHUB_REPOSITORY_OWNER != "apache")) {
+ // warn if publish was explicitly enabled but the version is a
snapshot or
+ // not from the ASF
+ if (publish && (is_snapshot || !is_apache)) {
core.warning("Publishing disabled for snapshot versions
and from non-apache repositories");
- publish = false;
}
Review Comment:
I wonder if we should improve this warning. As it is, it isn't really
helpful to know which of the issues disabled publishing, and it doesn't even
mention non-tagged commits as a possibility. Maybe move it up to after we
calculate do_publish and then do something like
```ts
if (!do_publish) {
core.warning("Publishing disabled:")
if (!publish) core.warning("- Published releases must set the 'publish'
setting to 'true'")
if (!is_tagged) core.warning("- Published releases must be triggered via a
tag")
if (is_snapshot) core.warning("- Published releases must have non-snapshot
versions")
if (!is_apache) core.warning("- Published releases must come from an ASF
repository")
}
```
I'm not sure the wording is great or if there's a better way to build the
warning, but something along those lines must be more clear. We can then remove
all the random places where we arning about disabling publishing since it's in
one spot.
##########
actions/release-candidate/dist/main/index.js:
##########
@@ -31855,46 +31855,59 @@ async function run() {
const tlp_dir = core.getInput("tlp_dir", { required: true });
const project_id = core.getInput("project_id", { required: true
});
const project_dir = core.getInput("project_dir");
- const gpg_signing_key = core.getInput("gpg_signing_key", {
required: true });
- const svn_username = core.getInput("svn_username", { required:
true });
- const svn_password = core.getInput("svn_password", { required:
true });
- const nexus_username = core.getInput("nexus_username", {
required: true });
- const nexus_password = core.getInput("nexus_password", {
required: true });
- let publish = core.getBooleanInput("publish");
-
- // import signing key into gpg and get it's key id
- let gpg_import_stdout = ""
- await exec("gpg", ["--batch", "--import", "--import-options",
"import-show"], {
- input: Buffer.from(gpg_signing_key),
- listeners: {
- stdout: (data) => { gpg_import_stdout +=
data.toString(); }
- }
- });
- const gpg_signing_key_id =
gpg_import_stdout.match("[0-9A-Z]{40}")[0];
- console.info("Using gpgp key id: " + gpg_signing_key_id);
+ const publish = core.getBooleanInput("publish");
+
+ // get the actual project version, this requires a 'VERSION'
file at
+ // the root of the repository
+ const project_version =
fs.readFileSync("VERSION").toString().trim();
+ const is_snapshot = project_version.includes("-SNAPSHOT");
+ const is_apache = process.env.GITHUB_REPOSITORY_OWNER ==
"apache";
+ const gitTagPrefix = "refs/tags/";
+ const is_tagged = github.context.eventName == "push" &&
github.context.ref.startsWith(gitTagPrefix);
+ const do_publish =
+ // Note that publishing could be disabled if the
publish input was explicitly set
+ // to false
+ publish
+ // require a pushed tag to enable publishing
+ && is_tagged
+ // require non-snapshot ASF builds to enable publishing
+ && (!is_snapshot && is_apache)
+ const gpg_signing_key = core.getInput("gpg_signing_key",
{required: do_publish});
+
+ let gpg_signing_key_id = "";
+ if (gpg_signing_key.trim() === "") {
+ // Generate keypair (non-interactive)
+ await exec("gpg", ["--batch", "--yes", "--passphrase",
'', "--quick-generate-key", process.env.USER ], {
+ silent: true
+ });
+ } else {
+ // import signing key into gpg
+ await exec("gpg", ["--batch", "--import",
"--import-options", "import-show"], {
+ input: Buffer.from(gpg_signing_key)
+ });
+ }
- // tags must be signed with a committers key, download and
import committer
- // keys for verification later
- let committer_keys = "";
- await exec("curl", [`https://downloads.apache.org/${ tlp_dir
}/KEYS`], {
+ // Capture the key id of the most recent generated/imported key
+ let gpg_list_secret_keys_stdout = "";
+ await exec("gpg", ["--list-secret-keys", "--with-colons"], {
silent: true,
listeners: {
- stdout: (data) => { committer_keys +=
data.toString(); }
+ stdout: (data) => {
+ gpg_list_secret_keys_stdout +=
data.toString().trim()
Review Comment:
I think trim() removes newlines, so if --list-secret-keys output multiple
lines they would all get squashed to a single line and break the split('\n')
below. Can we can just do `... += data.toString()` just in case? I imagine the
.split.findLast stuff should still work without trimmed content?
##########
actions/release-candidate/dist/main/index.js:
##########
@@ -31855,46 +31855,59 @@ async function run() {
const tlp_dir = core.getInput("tlp_dir", { required: true });
const project_id = core.getInput("project_id", { required: true
});
const project_dir = core.getInput("project_dir");
- const gpg_signing_key = core.getInput("gpg_signing_key", {
required: true });
- const svn_username = core.getInput("svn_username", { required:
true });
- const svn_password = core.getInput("svn_password", { required:
true });
- const nexus_username = core.getInput("nexus_username", {
required: true });
- const nexus_password = core.getInput("nexus_password", {
required: true });
- let publish = core.getBooleanInput("publish");
-
- // import signing key into gpg and get it's key id
- let gpg_import_stdout = ""
- await exec("gpg", ["--batch", "--import", "--import-options",
"import-show"], {
- input: Buffer.from(gpg_signing_key),
- listeners: {
- stdout: (data) => { gpg_import_stdout +=
data.toString(); }
- }
- });
- const gpg_signing_key_id =
gpg_import_stdout.match("[0-9A-Z]{40}")[0];
- console.info("Using gpgp key id: " + gpg_signing_key_id);
+ const publish = core.getBooleanInput("publish");
+
+ // get the actual project version, this requires a 'VERSION'
file at
+ // the root of the repository
+ const project_version =
fs.readFileSync("VERSION").toString().trim();
+ const is_snapshot = project_version.includes("-SNAPSHOT");
+ const is_apache = process.env.GITHUB_REPOSITORY_OWNER ==
"apache";
+ const gitTagPrefix = "refs/tags/";
+ const is_tagged = github.context.eventName == "push" &&
github.context.ref.startsWith(gitTagPrefix);
+ const do_publish =
+ // Note that publishing could be disabled if the
publish input was explicitly set
+ // to false
+ publish
+ // require a pushed tag to enable publishing
+ && is_tagged
+ // require non-snapshot ASF builds to enable publishing
+ && (!is_snapshot && is_apache)
Review Comment:
I'm not sure it makes sense for `!is_snapshot` and `is_apache` to be
combined like this anymore. They really are separate things we are checking for
and things make them feel related. Suggest we just do `&& !is_snapshot &&
is_apache`
##########
actions/release-candidate/dist/main/index.js:
##########
@@ -31950,14 +31974,42 @@ 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`);
- // enable SBT for publishing SBOM
+ // enable SBT for publishing SBOM either locally or remotely
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) {
+ let svn_config_dir = "";
+ if (do_publish) {
+ // if publishing is enabled, we must configure sbt to
write to a config file for
+ // post to read from
+ const svn_username = core.getInput("svn_username", {
required: true });
+ const svn_password = core.getInput("svn_password", {
required: true });
+
+ // Create the default config directory if it doesn't
exist
+ svn_config_dir = `${ os.homedir }/.subversion`;
Review Comment:
I think this can be const? It doesn't look like it's changed or used out
side of this block anymore
##########
actions/release-candidate/dist/post/index.js:
##########
@@ -125551,24 +125549,39 @@ async function run() {
}
}
- if (publish) {
+ if (do_publish) {
await exec("svn", ["add", artifact_dir]);
- await exec("svn", ["commit", "--username",
svn_username, "--password", svn_password, "--message", `Stage ${ project_name }
${ release_version }`, artifact_dir]);
+ await exec("svn", ["commit", "--message", `Stage ${
project_name } ${ release_version }`, artifact_dir]);
} else {
// if publishing was disabled then this action was
likely just triggered
// just for testing, so upload the maven-local and
artifact directories so
// they can be verified. Note that we do not just
recurse the
// release-download directory since it could contain
files that already
// exist in the SVN checkout and were not artifacts
created by this action
const release_dir = `${ os.tmpdir() }/release-download`;
+
+ const public_key_file = `${ release_dir
}/public-key.asc`;
+ // if publishing is disabled, store public key as
artifact so it can be downloaded
+ // by the post step for verification
+ let public_key = "";
+ await exec("gpg", ["--armor", "--export",
gpg_signing_key_id], {
+ silent: true,
+ listeners: {
+ stdout: (data) => {
+ public_key +=
data.toString().trim();
Review Comment:
I would recommend removing the trim(). That removes newlines which I don't
think breaks anything, but it makes the ascii armoring output non-standard.
--
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]
