-
Notifications
You must be signed in to change notification settings - Fork 3
VAULT-38191/use artifactory identity token in tests #34
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
Conversation
ryancragun
left a comment
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.
Looking good so far!
internal/artifactory/aql_test.go
Outdated
| WithUsername(vars["username"]), | ||
| WithToken(vars["token"]), | ||
| ) | ||
| if vars["username"] == "" { |
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.
You can probably simplify this a bit by creating a slice with the shared options, e.g.
opts := []Opt{
WithHost("https://artifactory.hashicorp.engineering/artifactory"),
WithToken(vars["token"]),
}
if vars["username"] != "" {
opts = append(opts, WithUsername(vars["username"]))
}
client = NewClient(opts...)398aeaf to
3e45f5f
Compare
62131fa to
6ce34cf
Compare
70ec99d to
9ec16e9
Compare
ryancragun
left a comment
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.
Lets clean up the docs and perhaps bump VERSION and this is g2g!
docs/resources/bundle_install.md
Outdated
|
|
||
| - `artifactory` (Object, Sensitive) - `artifactory.username` (String) The Artifactory API username. This will likely be your hashicorp email address | ||
| - `artifactory.token` (String) The Artifactory API token. You can sign into Artifactory and generate one | ||
| - `artifactory.bearer_token` (String) The Artifactory Identity token - use instead of username/ API token. You can get a token by joining the |
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 don't think this is right? We reuse token here and don't have an attribute named bearer_token in the schema.
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.
We'll want to remove to remove this change in resource_bundle_install and re-run the doc generator
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 dropped the commit that had these changes
| // criteria. | ||
| // | ||
| //nolint:paralleltest// because our resource handles it | ||
| func TestAccDataSourceArtifacotoryItemProperties(t *testing.T) { |
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.
Hey now, fixing typos that are like 4 years old.
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.
😎 🧹
|
|
||
| cfg := template.Must(template.New("enos_data_artifactory_item").Parse(`data "enos_artifactory_item" "vault" { | ||
| {{if .Username.Value -}} | ||
| username = "{{ .Username.Value }}" |
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.
Sorta surprised that this works. I would have expected us to require setting username to null when the value doesn't exist.
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.
Ah, the datasource has the username marked as optional, as it should: https://github.com/hashicorp-forge/terraform-provider-enos/blob/main/internal/plugin/datasource_artifactory_item.go#L208
| Description: docCaretToBacktick(` | ||
| - ^artifactory.username^ (String) The Artifactory API username. This will likely be your hashicorp email address | ||
| - ^artifactory.token^ (String) The Artifactory API token. You can sign into Artifactory and generate one | ||
| - ^artifactory.bearer_token^ (String) The Artifactory Identity token - use instead of username/ API token. You can get a token by joining the |
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.
Yeah, we should yank this. We ended up reusing token to be either the API key or the bearer token and didn't add a new attribute.
9ec16e9 to
7d21d06
Compare
…SourceArtifactoryItemProperties to handle token w no username
7d21d06 to
c89fb3e
Compare
.github/workflows/test.yml
Outdated
| - run: make test-acc | ||
| env: | ||
| ARTIFACTORY_USER: ${{ secrets.ARTIFACTORY_USER }} | ||
| ARTIFACTORY_TOKEN: ${{ secrets.ARTIFACTORY_TOKEN }} |
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.
are we using this anymore? or are we using both this and the bearer token?
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.
right now we're supporting both auth options: username/token and just bearer token
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 we remove the user and token and just use the bearer token before merge? That way we can delete the secrets and move on with our life?
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.
Good point! I removed them
How to read this pull request
VAULT-38191
Jfrog is no longer going to support username/ API key auth for Artifactory. Instead, an identity token (no username) will be used. Previous work has been done to support identity token auth in this repo (#31 & #22). This PR updates tests to use the identity token when there is no Artifactory user provided.
Note: The Artifactory identity token was added as
ARTIFACTORY_BEARER_TOKENin GH Actions secretsChecklist