-
Notifications
You must be signed in to change notification settings - Fork 29
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
feat: Add tokenless mutation and field in owner #826
base: main
Are you sure you want to change the base?
Conversation
@@ -37,4 +37,5 @@ type Owner { | |||
): RepositoryConnection! @cost(complexity: 25, multipliers: ["first", "last"]) | |||
username: String | |||
yaml: String | |||
tokensRequierd: Boolean |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, when would this field be set to Null instead of true/false?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep, when user is not part of the org (decorator rules)
|
||
self.validate(owner_obj) | ||
|
||
owner_obj.tokens_required = typed_input.tokens_required |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thoughts on this being token_required? And even to be more explicit, upload_token
required?
We use the github/providers token for other things, so differentiating this as the "codecov_token" or "upload_token" would make more sense. I also don't know if this exclusively a coverage token or if it also applies to bundle_analysis/test_results, so I'd confirm what's the expected behavior, and if it is exclusively for coverage (and we don't intend to do it for BA/TR), then I'd call it coverage_token
or upload_coverage_token
or something like that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is just a draft PR not the final code, just put tokens_required
as it's the suggested name by Trent for Nora, so i will change the name according to what it's called in the DB to avoid any confusion
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good! 👌
Purpose/Motivation
What is the feature? Why is this being done?
The user will set tokenless to either true or false. We need tokens_required to reflect the current status of tokenless within the organization, and we need a mutation to set tokenless to either required or not
this PR depends on:
Links to relevant tickets
[API] Expose new tokens_required field via graphql
[API] Add new require token mutation
What does this PR do?
Include a brief description of the changes in this PR. Bullet points are your friend.
Notes to Reviewer
Anything to note to the team? Any tips on how to review, or where to start?
Legal Boilerplate
Look, I get it. The entity doing business as "Sentry" was incorporated in the State of Delaware in 2015 as Functional Software, Inc. In 2022 this entity acquired Codecov and as result Sentry is going to need some rights from me in order to utilize my contributions in this PR. So here's the deal: I retain all rights, title and interest in and to my contributions, and by keeping this boilerplate intact I confirm that Sentry can use, modify, copy, and redistribute my contributions, under Sentry's choice of terms.