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

CI: Delete hardcoded GITHUB_TOKEN reference (Cirrus) #1235

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

DeeDeeG
Copy link
Member

@DeeDeeG DeeDeeG commented Feb 28, 2025

Change

Delete hardcoded GITHUB_TOKEN in Cirrus CI config file (.cirrus.yml)

Commentary (Rationale)

We can define this (encrypted) token on the web dashboard, instead of hard-coding (an encrypted reference to) it in a repo file, to save ourselves the trouble of updating it in the file every time it expires.

This token isn't particularly sensitive, as it has no permissions. It's only for authing as a real user so rate limits are relaxed, letting us download vscode-ripgrep from an otherwise heavily rate-limited datacenter / "cloud" machine that we call "CI."

For the more sensitive tokens, I'd like to be able to apply them per-step. Which I don't think the Cirrus web dashboard lets you do. This one should be alright to define globally.
(Heck, we already do define this one for the whole CI build file! Moving it to the web dashboard won't change that.)

Verification Process

I can trigger a Cron job on this branch, I guess.

Yep, it worked: https://cirrus-ci.com/task/4603233122910208

(ARM Linux needs some help with a newer Python version due to #1223. But it got past installing its npm deps, so this PR's fix worked there, too!)

UPDATE: ARM Linux Cirrus was fixed by #1237.

We can define this on the web dashboard instead to save ourselves the
trouble of updating it in the file every time it expires.

This token isn't particularly sensitive, as it has no permissions.
It's only for authing as a real user so rate limits are relaxed,
letting us download vscode-ripgrep from an otherwise *heavily*
rate-limited datacenter / "cloud" machine that we call "CI."

For the more sensitive tokens, I'd like to be able to apply them
per-step. Which I don't _think_ the Cirrus web dashboard lets you do.
This one should be alright to define globally.
(Heck, we already do define this one for the whole CI build file!
Moving it to the web dashboard won't change that.)
@DeeDeeG
Copy link
Member Author

DeeDeeG commented Mar 1, 2025

Okay, as it turns out, the web override does work, regardless of what the env var is defined as in the file.

So, this PR does not technically change anything, it's already being handled on the web dashboard, however it does serve as documentation, a reminder/hint to us as maintainers to do this on the web dashboard for Cirrus, not in the .cirrus.yml file, going forward.

We need to go to the dashboard to generate a new encrypted token either way, so not having to do a PR on top of that is very much preferable, IMO. So, this new paradigm (and documenting it with this PR) is my strong preference going forward.

But that just means merging this is optional. Again, this serves a just a docs improvement PR, basically, now that I have confirmed more details of how this works on the Cirrus side.

@DeeDeeG
Copy link
Member Author

DeeDeeG commented Mar 2, 2025

By the way (regarding this PR's own CI pass/fail status readout): I manually canceled the Apple Silicon Cirrus run to avoid spamming pulsar-rolling-releases with more bins. Apple Silicon Cirrus is passing now if I don't cancel it, since the web dashboard token override is working. 👍

And ARM Linux Cirrus runs are genuinely failing for reasons separate from this PR. The ARM Linux Cirrus runs need some sort of intervention to have a compatible Python version after the latest ppm bump #1223.

@@ -1,7 +1,9 @@
env:
PYTHON_VERSION: 3.12
GITHUB_TOKEN: ENCRYPTED[!587e64c0e1412de985721fd9a82605b5b359a3565854235f9b092824dd50c847e5ed3fcf34acf5ead8b2707cd50ec895!]
# The above token, is a GitHub API Token, that allows us to download RipGrep without concern of API limits
# GITHUB_TOKEN: ENCRYPTED[!587e64c0e1412de985721fd9a82605b5b359a3565854235f9b092824dd50c847e5ed3fcf34acf5ead8b2707cd50ec895!]
Copy link
Member

Choose a reason for hiding this comment

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

I think we can just remove this old value entirely, but I do agree with keeping a comment here documenting the use of a permissionless token here.

Copy link
Member Author

@DeeDeeG DeeDeeG Mar 7, 2025

Choose a reason for hiding this comment

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

I am drafting a commit to address this review feedback. Is one of the following preferable? Or something else?

  PYTHON_VERSION: 3.12
  # GITHUB_TOKEN: 
  # The above token, is a GitHub API Token, that allows us to download RipGrep without concern of API limits.
  # We define it on the Cirrus web dashboard now, though. That way we don't need to update repo files (this file),
  # or open any PR's and wait for review, just to update a permissionless token. (Expired tokens can block CI from passing/finishing!)

^ Just the old value removed.

  PYTHON_VERSION: 3.12
  # Note: the GITHUB_TOKEN env var holds a GitHub API Token, that allows us to download RipGrep without concern of API limits.
  # We define it on the Cirrus web dashboard now, though. That way we don't need to update repo files (this file),
  # or open any PR's and wait for review, just to update a permissionless token. (Expired tokens can block CI from passing/finishing!)

^ Deleting the line that had defined the env var in .cirrus.yml, and adjusting the text afterward to make sense.

Thanks for taking a look. 👍

Copy link
Member

@meadowsys meadowsys left a comment

Choose a reason for hiding this comment

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

presuming it's possible, I feel like we could remove all hardcoded secret references in this file and define them using the web dashboard

edit: I actually read through your comments now, lol. Not great that we can't apply the secrets per step if they're defined in the web dashboard, I'd definitely prefer the extra security over this convenience

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.

2 participants