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.

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

Behave well when MNCA isn't trusted #52

Merged
merged 5 commits into from
May 10, 2024
Merged

Behave well when MNCA isn't trusted #52

merged 5 commits into from
May 10, 2024

Conversation

grahamc
Copy link
Member

@grahamc grahamc commented May 10, 2024

Description
Checklist
  • Tested changes against a test repository
  • Added or updated relevant documentation (leave unchecked if not applicable)
  • (If this PR is for a release) Updated README to point to the new tag (leave unchecked if not applicable)

actionsCore.warning(TEXT_TRUST_UNTRUSTED);
return;
} else if (cacheAction.idslib.nixStoreTrust === "unknown") {
actionsCore.info(TEXT_TRUST_UNKNOWN);
Copy link
Member

Choose a reason for hiding this comment

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

Should this not be warning?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think so: it may be unknown if they're using a remote ssh store, for example. I don't want to emit useless warnings for scenarios we're not certain are worth a warning.

return;
}

if (cacheAction.idslib.nixStoreTrust === "untrusted") {
actionsCore.warning(TEXT_TRUST_UNTRUSTED);
Copy link
Member

Choose a reason for hiding this comment

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

Do we really want to exit with a warning instead of just erroring? If the runner user isn't trusted, then there's no point in having this action, and if that were me, I'd want to be notified that this is what happened (i.e. I personally would expect an error).

Copy link
Member Author

Choose a reason for hiding this comment

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

Philosophically I look at it like where we only warn and go into noop mode if Nix isn't installed. I'd rather not force the user to delete the action altogether if the trust situation is just temporary.

Copy link
Member

@cole-h cole-h left a comment

Choose a reason for hiding this comment

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

else lgtm

@cole-h
Copy link
Member

cole-h commented May 10, 2024

Well, the untrusted test failed, so probably want to fix that first (maybe make it required?).

@grahamc grahamc enabled auto-merge May 10, 2024 20:47
@grahamc grahamc merged commit 5555b8b into main May 10, 2024
6 checks passed
@grahamc grahamc deleted the untrusted branch May 10, 2024 20:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants