Fix output of remote step in nix workflow

[ckeshava]

This pull request updates the nix CI runner. Although the builds are successful, the binaries are not uploaded to the internal artifactory. This PR attempts to fix this issue and I have borrowed the idea from @ximinez

After successful authentication, outcome variable contains a string. In the upload step, we are checking if outcome == success as a prerequisite for uploading the binary.

This PR updates the contents of the outcome variable in the CI.

Example of a breaking Github Actions workflow (thanks to @thejohnfreeman): https://github.com/XRPLF/rippled/actions/runs/6264131645/job/17010134246

High Level Overview of Change

Context of Change

Type of Change

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• Refactor (non-breaking change that only restructures code)
• Tests (You added tests for code that already exists, or your new feature included in this PR)
• Documentation Updates
• Release
• None of the above
  This does not modify any of the source code files. The CI runner condition is updated to ensure uploads of binaries upon successful builds.

This change as motivated from this slack discussion: https://ripple-enterprise.slack.com/archives/C02GE9NBA/p1695051230548129

[ckeshava]

Is there any way I can try out the CI changes in this PR? How can I ensure that these commands work correctly?

[thejohnfreeman]

What do you mean the binaries are not uploaded? It appears to me that they are. How would you be able to tell if you could not inspect the Artifactory?

The Artifactory is public and browsable without credentials. There you can see the list of packages and their binaries.

[thejohnfreeman]

I'll approve this change because it improves the observability of the workflow. I don't think the title or description of this PR is accurate, though.

[intelliot]

I don't think the title or description of this PR is accurate, though.

@thejohnfreeman could you suggest an alternate title and description?

[ckeshava]

@thejohnfreeman This was an observation made by @ximinez and @godexsoft , here is the context of the discussion: https://ripple-enterprise.slack.com/archives/C02GE9NBA/p1695084567770189?thread_ts=1695051230.548129&cid=C02GE9NBA

I am of the understanding that the Github CI was not correctly returning success, which caused the binaries to not be uploaded to the internal artifactory.

[thejohnfreeman]

I know the thread. I commented in it. It has 180 comments, though, and I don't feel like digging through all that in the Slack UI to try to figure out what you mean in the description here. Instead of linking to the Slack (which is not visible to non-Ripple employees), it would be better to link to a job run that you think failed to upload correctly (which is visible).

I looked and found one. The problem is that the step output starts with the string "Changed user of remote ..." printed by the conan user command.

This PR does not change the condition for upload. It fixes the output of the remote step.

Recommended:
Title: Fix output of remote step in nix workflow
Description: Let conan user command print to terminal.

[ckeshava]

@thejohnfreeman thanks, this helps.
How did you search for the job which failed at the upload step? Is there a search/filter option amongst the jobs or do we need to manually eye-ball it?

Did we face issues with binary uploads anytime prior to this week? This issue must have surfaced before.
Alternatively, if one of the developers manually uploaded the binaries, we wouldn't have noticed it.

[thejohnfreeman]

I filtered for the nix workflow specifically, but then eyeballed for a commit that mentioned a change to the dependencies.

[ximinez]

The problem with creating your branch in the XRPLF repo is that it has those secrets configured, so the connection works. On a personal repo (or just one that doesn't have secrets), when the conan user command fails, the whole step fails. https://github.com/ximinez/rippled/actions/runs/6342827288/job/17229389904

There's an easy fix, though. The shell is running with set -e, so you can undo it by starting the run: with set +e.

Example from my Windows CI PR: https://github.com/XRPLF/rippled/actions/runs/6343277722/job/17230836193?pr=4596#step:9:28

[ckeshava]

Thanks for pointing this out.
I have incorporated your suggestions in this commit (ckeshava-patch-2...ckeshava:rippled:ckeshava-patch-2), but I don't have permissions to update this branch, because this is a branch on XRPLF's fork.

Do I need to create a new PR?

[thejohnfreeman]

I think we've fixed this in other places with || true, which is the fix I prefer because it is highly specific. Only one command is permitted to fail.

[ximinez]

|| true will change the value of $?, won't it? That would defeat the whole point of this PR.

$ false ; [ $? -eq 0 ] && echo success || echo failure
failure

$ false || true ; [ $? -eq 0 ] && echo success || echo failure
success

[ximinez]

I don't have permissions to update this branch, because this is a branch on XRPLF's fork.

How did you create the branch in the first place?

[ximinez]

@ximinez
If the secrets are not configured, then outcome is set to failure in both the suggestions. Won't both of these commands stop the build process?

No. That's the reason it's written this way. Setting outcome=failure only affects the "upload dependencies to remote" and "recreate archive with dependencies" steps.

[ckeshava]

Ah, the additional tee ${GITHUB_OUTPUT} in your suggestion protects the entire bash command from failure. I'm guessing the command will short-circuit to the next OR condition. I think I understand it now 👍

[ximinez]

Yet another option. A minimal change to the original. Just redirects output of conan user to stderr.

echo outcome=$(conan user --remote ripple ${{ secrets.CONAN_USERNAME }} --password ${{ secrets.CONAN_TOKEN }} >&2 && echo success || echo failure) | tee ${GITHUB_OUTPUT}

This sounds like a winner.

[ckeshava]

I don't have permissions to write into this branch :/
let me ask elliot and revert back to this PR

[intelliot]

I don't have permissions to update this branch, because this is a branch on XRPLF's fork.

How did you create the branch in the first place?

@ckeshava previously had write access, but this was changed to read in order to improve the security of this repo. In general, contributions should be developed in your personal fork.

@ckeshava : please open a new PR using a branch on your fork. Thanks!

[ckeshava]

this PR is superceded by #4746