Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix output of remote step in nix workflow #4716

Closed
wants to merge 2 commits into from
Closed

Fix output of remote step in nix workflow #4716

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7145617
Update nix.yml
ckeshava Sep 19, 2023
612d427
rewording of the command for uploading artifacts
ckeshava Sep 21, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion .github/workflows/nix.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,8 @@ jobs:
- name: try to authenticate to ripple Conan remote
id: remote
run: |
echo outcome=$(conan user --remote ripple ${{ secrets.CONAN_USERNAME }} --password ${{ secrets.CONAN_TOKEN }} && echo success || echo failure) | tee ${GITHUB_OUTPUT}
conan user --remote ripple ${{ secrets.CONAN_USERNAME }} --password ${{ secrets.CONAN_TOKEN }}
echo outcome=$([ $? -eq 0 ] && echo success || echo failure) | tee ${GITHUB_OUTPUT}
Comment on lines 79 to +83
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

|| 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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 👍

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

@ckeshava ckeshava Oct 2, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

- name: archive profile
# Create this archive before dependencies are added to the local cache.
run: tar -czf conan.tar -C ~/.conan .
Expand Down