stevedlawrence commented on code in PR #20:
URL:
https://github.com/apache/daffodil-infrastructure/pull/20#discussion_r2491872109
##########
actions/release-candidate/action.yml:
##########
@@ -36,20 +36,20 @@ inputs:
required: true
svn_username:
description: Username for publishing release artifacts to SVN dev/dist
- required: true
+ required: false
svn_password:
description: Password for publishing release artifacts to SVN dev/dist
- required: true
+ required: false
nexus_username:
description: Username for publishing release artifacts to Nexus
- required: true
+ required: false
nexus_password:
description: Password for publishing release artifacts to Nexus
- required: true
+ required: false
publish:
description: Enable/disabling publish artifacts. Must be explcitly set to
true to enable publishing. Maybe ignored depending on other factors.
required: false
- default: false
+ default: "false"
Review Comment:
Just curious, is this a thing that the value of `default` should always be
quoted?
##########
actions/release-candidate/dist/main/index.js:
##########
@@ -31856,11 +31856,11 @@ async function run() {
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");
+ let publish = core.getBooleanInput("publish");
+ const svn_username = core.getInput("svn_username", { required:
publish });
+ const svn_password = core.getInput("svn_password", { required:
publish });
+ const nexus_username = core.getInput("nexus_username", {
required: publish });
+ const nexus_password = core.getInput("nexus_password", {
required: publish });
Review Comment:
I would recommend we don't require these settings based it on the 'publish'
setting. The reason is that it's useful to test the workflow without changing
it, but by tagging with a -SNAPSHOT tag or using workflow dispatch. In those
case, the workflow will will disabling publishing even if the publish setting
is true.
Instead, I would suggest that we just get these specific inputs when we are
actually going to use them. For example, instead of accessing nexus_* here, we
instead modify the `if (publish)` block to get those inputs and require that
they exist. E.g.
```ts
if (publish) {
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 });
// modify build.sbt and publish config files with auth information
...
} else {
...
}
```
This way we only ever try to access those settings if we are only going to
actually publish things. And if they don't exist then the workflow will fail.
##########
actions/release-candidate/README.md:
##########
@@ -167,16 +167,10 @@ Perform the following steps to test changes to the
daffodil-infrastructure repo
`.github/workflows/release-candidate.yml` file of the repository to be
tested on. Then,
push your changes to your fork of the test repository.
2. Add the secrets required by the `ASF Release Candidate` step to your
- test repository. The following secrets are required:
+ test repository. The following secret is required:
- DAFFODIL_GPG_SECRET_KEY (any private key without a passphrase)
Review Comment:
What are your thoughts on making `DAFFODIL_GPG_SECRET_KEY` also optional if
not publishing? It would make testing this so much easier if we don't require
any secrets at all, especial gpg keys which are kindof annoying to
generate/export/etc.
Maybe if publishing is disabled then we don't require the input. We use the
value if it's not empty, but if it is empty then we generate a GPG key
public+secret pair and sign with the secret. That way all the stuff like `sbt
publishSigned` will still work correctly. We could also extract and upload the
public key as another artifact when publishing is disabled in post.js so that
users could download and import the key if they wanted to verify signatures.
Also, kindof related to gpg signatures, we currently verify that the git tag
was created by a committer:
```ts
// 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]);
```
Maybe we should also move that into the `if (publish)` true block so that
non-committers (or committers that haven't created keys yet) can test creating
releases. I imagine that won't happen very often, but it simply enough to
support.
##########
actions/release-candidate/dist/post/index.js:
##########
@@ -125521,12 +125521,12 @@ const { exec } = __nccwpck_require__(95236);
async function run() {
try {
const project_name = core.getInput("project_name", { required:
true });
- const svn_username = core.getInput("svn_username", { required:
true });
- const svn_password = core.getInput("svn_password", { required:
true });
const artifact_dir = core.getState("artifact_dir");
const gpg_signing_key_id = core.getState("gpg_signing_key_id");
const publish = core.getState("publish") === "true";
+ const svn_username = core.getInput("svn_username", { required: publish
});
+ const svn_password = core.getInput("svn_password", { required: publish
});
Review Comment:
Same idea here as above, we should move the svn getInputs into the below `if
(publish)` block.
--
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]
