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

Move REPO_TOKEN to CML_ environment variable namespace #763

Closed
casperdcl opened this issue Oct 12, 2021 · 28 comments · Fixed by #1272
Closed

Move REPO_TOKEN to CML_ environment variable namespace #763

casperdcl opened this issue Oct 12, 2021 · 28 comments · Fixed by #1272
Assignees
Labels
cml-runner Subcommand discussion Waiting for team decision good-first-issue Good for newcomers (good-first-issue) p1-important High priority technical-debt Refactoring, linting & tidying ui/ux User interface/experience

Comments

@casperdcl
Copy link
Contributor

casperdcl commented Oct 12, 2021

Use CML_TOKEN (or similar, e.g. CML_CI_TOKEN etc.) instead of REPO_TOKEN to

  • avoid potential namespace clashes
  • better reflect that it could be a project token, org token, bot token, encoded username & password, etc
  • better reflect that it's mostly used to launch runners and also is used by runners

@casperdcl casperdcl changed the title namespace config CML_ env namespace Oct 12, 2021
@casperdcl casperdcl transferred this issue from iterative/cml.dev Oct 12, 2021
@casperdcl casperdcl added cml-runner Subcommand good-first-issue Good for newcomers (good-first-issue) p2-nice-to-have Low priority technical-debt Refactoring, linting & tidying ui/ux User interface/experience labels Oct 12, 2021
@0x2b3bfa0
Copy link
Member

Closed #764 because violets are red. Do you have any brilliant solution to this issue?

@DavidGOrtega
Copy link
Contributor

DavidGOrtega commented Oct 13, 2021

CML_TOKEN is misleading, sounds like if we had a token to be set instead of the SCM_TOKEN

@casperdcl
Copy link
Contributor Author

CML_TOKEN is misleading

do you have a better suggestion?

@DavidGOrtega
Copy link
Contributor

DavidGOrtega commented Oct 13, 2021

currently we do no have an issue with repo_token, but maybe scm_token? the user will know that we do not require anything on our side, just only in the SCM side. As I say CML_TOKEN sounds that we are the owners of that token.
Don't you feel the same?

@casperdcl
Copy link
Contributor Author

Don't feel the same.

  • firstly it's about people users wanting to label things based on their use rather than ownership (i.e. "I'm using this token for my CML operations," not GitHub Actions etc. operations)
  • secondly it's about... use. It is almost never used for SCM. cml-pr is the only exception. In all other cases it's used for API calls - connecting launched runners to the CI, pushing comments, etc.

@dacbd
Copy link
Contributor

dacbd commented Oct 13, 2021

GIT_PROVIDER_TOKEN of SCM_API_TOKEN?

I feel CML_TOKEN is misleading but agree that SCM_TOKEN isn't clear about its use.

Without being familiar with all the supported environments or introducing unneeded complexity, maybe something like GITHUB_API_TOKEN, BITBUCKET_API_TOKEN, GITLAB_API_TOKEN as parsed values for their respective envs which is collapsed down to SCM_API_TOKEN (possibly prefixed with CML_)

@casperdcl
Copy link
Contributor Author

CML_CI_TOKEN?

@DavidGOrtega
Copy link
Contributor

CML_REPO_TOKEN?

@dacbd
Copy link
Contributor

dacbd commented Oct 16, 2021

So this is basically the portion we are referring to:

cml/src/cml.js

Lines 36 to 47 in 766aac8

const inferToken = () => {
const {
REPO_TOKEN,
repo_token: repoToken,
GITHUB_TOKEN,
GITLAB_TOKEN,
BITBUCKET_TOKEN
} = process.env;
return (
REPO_TOKEN || repoToken || GITHUB_TOKEN || GITLAB_TOKEN || BITBUCKET_TOKEN
);
};

and the goal is to have an ENV name that the user could specify, that intuitively describes its use and isn't at risk of getting clobbered by some CI system? like with RUNNER_NAME

and this CML_***** env would be used over the token inferred by the above snippet.

@0x2b3bfa0
Copy link
Member

0x2b3bfa0 commented Oct 6, 2022

Footnotes

  1. What @DavidGOrtega, @casperdcl and @dacbd really meant with attrocities like GIT_PROVIDER_TOKEN, SCM_API_TOKEN,
    CML_CI_TOKEN or CML_REPO_TOKEN in previous comments. 🐲

@0x2b3bfa0
Copy link
Member

I'd suggest just replacing REPO_TOKEN by CML_TOKEN in all our documentation and examples.

🔔 @iterative/cml, please vote 👍🏼 👎🏼 on this comment.

@0x2b3bfa0 0x2b3bfa0 changed the title CML_ env namespace Move REPO_TOKEN to CML_ environment variable namespace Oct 6, 2022
@casperdcl
Copy link
Contributor Author

currently still leaning towards CML_CI_TOKEN

@0x2b3bfa0
Copy link
Member

And how does CI describe the API that token is used for? 🤔

@dacbd
Copy link
Contributor

dacbd commented Oct 7, 2022

because that token is needed by all the actions we preform in the CI system?

@0x2b3bfa0
Copy link
Member

That's also the case with all the other tokens. 🤔 They're usually not named after the system that uses them, but after the system that requires them for authentication.

E.g. if a token is used to authenticate with a cloud, it's a “cloud token” and not a “computer token” even if you use it from a computer. 😅

@dacbd
Copy link
Contributor

dacbd commented Oct 7, 2022

😄 it's more descriptive of what it is used for than CML_TOKEN is; cml does not have an API your need to interact with so why does it need a token to do anything??? CI is at least a fill-in for Github Actions/GitLab-CI/BB pipelines and you know the provider its making calls to. I understand your insistence to use the term forge, but it is still not commonplace.

If the issue is handling the chance for a collision just silently keep CML_TOKEN then namespace REPO_TOKEN that's what users know already and will be easier (mentally) to move to. If we want to have naming holy wars, we should have handled this with the command renaming.

I'd argue for the moment there's no problem to fix so let's not make it one...

@0x2b3bfa0
Copy link
Member

[CML_CI_TOKEN] is more descriptive of what it is used for than CML_TOKEN is

Indeed, it has more words, although whether they help clarifying its purpose or not is arguable 😅

cml does not have an API your need to interact with so why does it need a token to do anything?

I fell on my own trap of #763 (comment); indeed, tokens should not be named after the system that uses them, but after the system that validates them. 🙈

CI is at least a fill-in for Github Actions/GitLab-CI/BB pipelines and you know the provider its making calls to

That's really contorted. The token is being used from the continuous integration system, to authenticate against the forge API. 🙃

I understand your insistence to use the term forge, but it is still not commonplace.

I don't mind if we use “forge” or not, but CI is by far less appropriate than forge in this context. That token is not [only/really] being used to authenticate to the CI API.

If the issue is handling the chance for a collision just silently keep CML_TOKEN then namespace REPO_TOKEN that's what users know already and will be easier (mentally) to move to.

Also arguably, REPO_TOKEN is an equally obtuse name: it's not scoped to a single repository, is not [only/mainly] used to access the [Git] repository, and “repo” is not a service you can authenticate against.

If we want to have naming holy wars, we should have handled this with the command renaming.

Too easy. Nobody will leave this thread alive. ⚔️

I'd argue for the moment there's no problem to fix so let's not make it one...

Documentation, probably. Nothing is broken in the sense that it doesn't work, but...

@dacbd
Copy link
Contributor

dacbd commented Oct 9, 2022

CI is at least a fill-in for Github Actions/GitLab-CI/BB pipelines and you know the provider its making calls to

That's really contorted. The token is being used from the continuous integration system, to authenticate against the forge API. 🙃

A large portion of the token's usage is to register the CI Agent, and in terms of GitHub, the only case where a PAT is actually required. the GITHUB_TOKEN can be given appropriate permissions to perform all of the other actions.

If the issue is handling the chance for a collision just silently keep CML_TOKEN then namespace REPO_TOKEN that's what users know already and will be easier (mentally) to move to.

Also arguably, REPO_TOKEN is an equally obtuse name: it's not scoped to a single repository, is not [only/mainly] used to access the [Git] repository, and “repo” is not a service you can authenticate against.

Being equally obtuse isn't a good enough reason to change it since its use is already established in documentation, blog posts, and [video] tutorials.

If we want to have naming holy wars, we should have handled this with the command renaming.

Too easy. Nobody will leave this thread alive. ⚔️

😢 but true 🤺

I'd argue for the moment there's no problem to fix so let's not make it one...

Documentation, probably. Nothing is broken in the sense that it doesn't work, but...

Our Documentation is already behind in some updates, so I don't see the value in adding more to it.
It working, is the definition of not broken IMO. If we all agree to disagree on this point then maybe we can schedule the great CML re-write to python or maybe some rust

@0x2b3bfa0
Copy link
Member

A large portion of the token's usage is to register the CI Agent

Yes, roughly 1 command out of 7. 🤨😉

and in terms of GitHub, the only case where a PAT is actually required, the GITHUB_TOKEN can be given appropriate permissions to perform all of the other actions

Tangential but true. Note that CML supports three different forges, and the token we're asking users to provide is being used for every operation, no matter which kind of token it is.

Being equally obtuse isn't a good enough reason to change it since its use is already established in documentation, blog posts, and [video] tutorials.

Almost agreed, in the sense that changing the documentation for such an insignificant detail is cumbersome. Still, that probably means that our documentation processes should be smoother; we'll always have to amend lots of bad API choices, and the earlier, the better. Sounds easier to fix 1 post and 4 videos now than 2 posts and 8 videos in six months. 1

😢 but true 🤺

Don't forget your hauberk! 😄

Our Documentation is already behind in some updates, so I don't see the value in adding more to it.

We can probably add it just as a reminder.2

It working, is the definition of not broken IMO.

That's why we're fixing it. 😉

If we all agree to disagree on this point then maybe we can schedule the great CML re-write to python or maybe some rust

Absolutely Wright. I was hoping for Brain... well, that language, but that's probably out of scope for this issue. Rust seems hard to justify, but Python would simplify quite a few integrations.

Footnotes

  1. A healthy extreme programming paradign relies on frequent and smooth refactoring; if it's difficult, we should ask ourselves why

  2. An issue tracker should be a priority list, rather than a checklist; “adding” is not the same operation as “prioritizing” in the sense that “not adding by fear of overwhelming” is harmful, but “not prioritizing” is just arguable at worst

@dacbd
Copy link
Contributor

dacbd commented Oct 9, 2022

Yes, roughly 1 command out of 7. 🤨😉

In proving my point I would dispute your interpretation of these numbers; you can say I'm cherry-picking, but looking at non-error invocations, it is closer 1/4. As well I would further discount publish usage as most cases there are multiple uses for a single send-comment making up what, a user and us by way of #1208, consider to be a single action. Additionally, 99% of the time publish uses a public API to function, not requiring any token.

@dacbd
Copy link
Contributor

dacbd commented Oct 9, 2022

I would also like to state that renaming !== refactoring

@casperdcl
Copy link
Contributor Author

casperdcl commented Oct 12, 2022

Given we've agreed not to ever use the word "forge" in any context ever, CML_GITHOST_TOKEN?

I still think CI is fine because even GH doesn't really distinguish between "git provider" and "CI" (citation: https//api.github.com being used for both purposes, and the token perms including CI-specific functionality)

@0x2b3bfa0
Copy link
Member

Given we've agreed not to ever use the word "forge" in any context ever[citation needed]

Arguable as per comments linked from #1142 but, indeed, we can try not to use it.

CML_GITHOST_TOKEN

I'm allergic to cache invalidation; worth visiting #1142 to find a generic name for GitHub/GitLab/Bitbucket if we really need one? 🤧

@0x2b3bfa0
Copy link
Member

I still think CI is fine because even GH doesn't really distinguish between "git provider" and "CI"

Following that rule of thumb, we can use CML_WIKI_TOKEN or any other name related to a fraction of the functionality exposed by the forge API.

@0x2b3bfa0
Copy link
Member

0x2b3bfa0 commented Oct 12, 2022

I still haven't found alternatives (subjectively) better than CML_TOKEN because:

  • It skillfully avoids naming forges as such, nor by any other “creative” name.
  • Is already parsed by yargs as expected.

@casperdcl
Copy link
Contributor Author

CML_DRIVER_TOKEN? :)

@0x2b3bfa0
Copy link
Member

CML_DRIVER_TOKEN

This would reduce this problem to “just” #1142

@0x2b3bfa0
Copy link
Member

Opened #1272 as per #763 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cml-runner Subcommand discussion Waiting for team decision good-first-issue Good for newcomers (good-first-issue) p1-important High priority technical-debt Refactoring, linting & tidying ui/ux User interface/experience
Projects
None yet
4 participants