-
Notifications
You must be signed in to change notification settings - Fork 103
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
✨ Add private registry provider, provider version, and provider platform APIs #313
Conversation
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.
All in all, everything looks awesome so far 🔥 ! Admittedly, I haven't given it a thorough review but a few 1.0 related things stuck out to me looking over your changes.
One thing I'll add is if you add any include
params to a read or list option to make sure the valid relations that can be included live as string enums #294
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 Karl!
I left two comments for you.
I am happy to see that your PR is following our go-tfe best practices so far.
Once your PR is no longer a draft and you are ready for a final review, I'd like to smoke test your changes. If you can leave in the PR description detailed instructions on how to smoke test for this feature, I'd greatly appreciate it!
Hi @joekarl , @sebasslash , @uturunku1 We are very much interested in publishing providers to private registry. Do I understand right that this MR is the missing piece for it? |
546d408
to
a26c40e
Compare
Hi @daniel-laszlo ! Yes if your means is to use go-tfe, however in the meantime you can do so directly calling the private registry/provider API. Once the go-tfe changes are released, we'll subsequently add support to the |
285f853
to
09ff03f
Compare
338ef61
to
e2dd192
Compare
e2dd192
to
a723a6b
Compare
ec7f2f3
to
40a3165
Compare
2ff8259
to
005dfbf
Compare
I've been thinking about breaking this PR up into 3 separate PRs, and I think I'll do that. |
005dfbf
to
1f41bf6
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.
Submitting a partial review. I still need to review the tests. Looks great!
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 great! Almost there across the finish line 🐎
e98b091
to
bf28899
Compare
ad5d732
to
4a9b53b
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.
@annawinkler Hey Anna! If you have any questions about my comments, let me know over slack (I occasionally miss pings over GitHub). Great job with all the testing done by the way!
4a9b53b
to
42b8284
Compare
A question was raised about these methods - should they be exported? I don't think they should be. |
Actually, we do want to export them. The idea is the workflow would be:
|
5bd53d4
to
913d0e3
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.
Very nice
t.Run("with valid options", func(t *testing.T) { | ||
options := RegistryProviderPlatformCreateOptions{ | ||
OS: "foo", | ||
Arch: "scrimbles", |
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, yes, scrimbles64
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.
Lol, I couldn't find the meaning of "scrimbles" in the dictionary, but this sounds funny 😆
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.
It's a made up word from a former coworker from long ago that I still use 🥳
19bae4c
to
eb1c817
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.
Woohooo @annawinkler and @joekarl !!! :celebration_dance
eb1c817
to
3b146b7
Compare
3b146b7
to
7392512
Compare
Reminder to the contributor that merged this PR: if your changes have added important functionality or fixed a relevant bug, open a follow-up PR to update CHANGELOG.md with a note on your changes. |
Description
This PR adds support for private registry provider APIs, which includes adding support for the following APIs:
Testing plan
To test these changes locally, create a local go project, and in the
go.mod
include a line similar to the following:Then, you can test the following functionality:
External links
Output from tests (HashiCorp employees only)