-
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
no code provisioning in registry modules #562
Conversation
Co-authored-by: UKEME BASSEY <[email protected]>
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!
…o-tfe into miguelhrocha/no-code-modules
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.
Hi @miguelhrocha - thank you for your PR! 🥳 Could you please add a changelog entry for this update? We have contributing guidelines here that might be helpful. Thanks!
Thanks for reaching out @annawinkler ! Sorry for missing this, I've updated the PR b1e5d0c 👍🏼 |
Hi @miguelhrocha - when I tested with the example you included in the PR description, I saw this error:
Based on what I'm reading in the docs, I would still expect this error, so in my test, I changed: |
namespace := url.QueryEscape(moduleID.Namespace) | ||
name := url.QueryEscape(moduleID.Name) | ||
provider := url.QueryEscape(moduleID.Provider) | ||
url := fmt.Sprintf("organizations/%s/registry-modules/%s/%s/%s/%s", org, registryName, namespace, name, provider) |
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.
💅 A small style suggestion - in this file (& the rest of the code), we generally avoid creating variables for each URL element like on this line.
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 think we're almost there - I left a few more suggestions.
I'll be out and I don't want to block this PR. I think it's close!
🥳 I tested locally with a local tfc, and verified that if I create one Registry Module with no code = true, and one with no code = false, when I look at the API scope |
💅 One final request - please squash merge the commits into one commit. Thanks! |
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.
The code here looks great 🚀 . There is one minor non-blocking comment below ⬇️
I may even go so far as to say if you can squash your 10 commits to 3 (if possible): one for your implementation, one for your tests, and one for your changelog/housekeeping changes. You don't have to follow this structure but it helps keep our history clean 😄
Hi @sebasslash ! Thanks for reviewing my code 😄 I had to resolve some conflicts so I implemented your proposal. I will use the merge squash functionality from GitHub 👍🏼 |
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.
Changes look good! 🚀 Feel free to merge when you're ready 👍
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 change aligns with the next release of TFE which will include no-code modules provisioning introduced on TFC during the last HashiConf.
The way to create these modules is to either enable this property at the time of creation or with the newly introduce update operation.
Testing plan
Using integration tests
Using TFE instance
External links
Output from tests
Including output from tests may require access to a TFE instance. Ignore this section if you have no environment to test against.
Update endpoint changes
Create endpoint changes