-
Notifications
You must be signed in to change notification settings - Fork 339
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
Filter registry credentials by language #2680
Conversation
4263e13
to
0e7e3db
Compare
0e7e3db
to
e02d65a
Compare
d9e8b07
to
de0f9cf
Compare
src/start-proxy-action.ts
Outdated
@@ -17,6 +17,12 @@ const PROXY_USER = "proxy_user"; | |||
const KEY_SIZE = 2048; | |||
const KEY_EXPIRY_YEARS = 2; | |||
|
|||
const LANGUAGE_TO_REGISTRY_TYPE = { | |||
"java-kotlin": "maven_repository", |
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.
Should you add kotlin
as a standalone language?
Also there is languages.ts
, which is where we've traditionally added language-specific logic. I think I'd prefer to move this declaration to that file.
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.
I've pushed a commit where this logic better integrates with languages.ts
, by reusing the Language
type defined there.
I am 50/50 on whether we should move this map there. The main reason against doing so is that this information is really only useful for the proxy action and even the names of the registry types are fairly arbitrary and dependent on the specifics of the proxy binary. WDYT?
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.
I'm fine if the logic doesn't go into language.ts.
f33e08c
to
529f92f
Compare
529f92f
to
31d11b1
Compare
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.
Looks good. I have a suggestion, but it is purely stylistic and non-blocking.
actions: "", | ||
cpp: "", | ||
go: "", | ||
swift: "", |
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.
nit:
actions: "", | |
cpp: "", | |
go: "", | |
swift: "", | |
actions: undefined, | |
cpp: undefined, | |
go: undefined, | |
swift: undefined, |
const registryTypeForLanguage = language | ||
? LANGUAGE_TO_REGISTRY_TYPE[language] | ||
: undefined; |
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.
nit: If you apply the suggestion above, then this becomes simpler:
const registryTypeForLanguage = language | |
? LANGUAGE_TO_REGISTRY_TYPE[language] | |
: undefined; | |
const registryTypeForLanguage = LANGUAGE_TO_REGISTRY_TYPE[language]; |
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.
I still need the check on language
itself, though.
Introduce the
language
parameter for the start-proxy action so we know which credentials we should use.By specifying the language with disambiguating cases where the same registry has distinct credentials. For example, in Artifactory it is possible to use a token that is scoped to the nuget feed, while the maven repository uses the username/password combination.
Merge / deployment checklist