-
Notifications
You must be signed in to change notification settings - Fork 38
Conversation
0461986
to
5dcead1
Compare
b8c4612
to
142cc63
Compare
5efdb6e
to
323226e
Compare
I tried creating a provider and configuring a resource in the provider, following the steps in the documentation. I completed the tests without any problems. These documents are clear to me. |
Thanks for trying it out @sergenyalcin, much appreciated! |
|
||
Run: | ||
```bash | ||
go mod tidy |
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 may be helpful to note that providers that are >v1 without being compatible with https://go.dev/blog/v2-go-modules will need to update their go.mod
and set the version of their provider with the hyphenated format (I don't know what to call this format and I don't know how to get it back from a command - I always dig up and truncate the commit sha and timestamp by hand).
After this, they will need to run go mod tidy
again.
$ go mod tidy
go: downloading github.com/equinix/terraform-provider-metal v1.1.1-0.20210929123308-2d7fb2c4b0ce
mkdir config/branch | ||
``` | ||
|
||
```bash |
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.
```bash | |
```go |
Because they won't write the exact code, seeing it in command format won't really help as much. So, I'd suggest having YAMLs and Go snippets as is to get syntax highlighting and get users to copy-paste them in their editors instead of running commands. The same doesn't go for the generic find&replace commands we have at the top for example, because they will run them regardless of the provider they're generating.
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.
these files don't exist at this point and I found this to be more convenient while following the guide, instead of creating directory, file and copy/pasting file content to an editor.
"github_repository$", | ||
"github_branch$", |
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.
"github_repository$", | |
"github_branch$", | |
"github_repository", | |
"github_branch", |
Would it be easier to understand if we targeted single resources here instead of having prefixes?
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.
Including a commented out .*
example would demonstrate that regex is available and showcase (what I imagine) would be the most common setting.
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 is difficult to exclude patterns. Perhaps a WithExcludeList feature (or equivalent example) would help.
This comes in handy when you don't want to persist your deprecated Terraform resources types.
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.
nm - I found WithSkipList
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.
@muvaf not sure I got your point. We are already including single resource when we append $
to end. Otherwise, it will generate all resources starting with github_repository
or github_branch
})) | ||
``` | ||
|
||
7. Finally, we would need to add some custom configurations for these two |
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.
Because the custom config doc goes into great detail, what do you think leaving provider generation part end here? i.e. use .*
to generate all and then let them know they can now provide custom configs by following that other guide.
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 was about to leave the same comment.
I'm about to run through this guide for a second provider and I plan on skipping steps 4, 6, and 7. I'll use .*
.
Once I have all of the CRDs generated (and pushed), I will revisit step 7 to set up references. This will be an open issue on my new project "setup referencers".
I'll revisit step 6 if I have to remove any resources that don't convert well.
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 would prefer to create an end-to-end experience here.
Starting from scratch and having some resources provisioned. I am a bit hesitant that if people try to generate all resources at this point, they might be discouraged due to some unexpected issues (e.g. conflicts due to a missing resource configuration).
I've extended the note after step 8 to mention how all resources could be generated and configuring them could be a next step.
- [Additional Sensitive Fields and Custom Connection Details] | ||
- [Late Initialization Behavior] | ||
|
||
### External Name |
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 this section needs an update because of #132
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.
Opened a ticket for that: #153
docs/configuring-a-resource.md
Outdated
have some custom configuration API that would allow marking additional fields as | ||
sensitive (e.g. just if we encounter a field that is not marked properly) and |
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 the current API does not allow marking as sensitive, it allows only adding more keys to the secret. Maybe a small note somewhere saying if the schema did not mark it as sensitive and it shows up in CRD, you can make it sensitive by changing 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.
I believe it is quite exceptional and doesn't worth adding a note here. To the best of my knowledge, we are not aware of such a case yet, right?
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.
Maybe, a dedicated section detailing the possibility and reasoning of overriding terraform schema could be added as part of #153
Terrajet runtime automatically performs late-initialization during | ||
an [`external.Observe`] call with means of runtime reflection. | ||
State of the world observed by Terraform CLI is used to initialize | ||
any `nil`-valued pointer parameters in the managed resource's `spec`. |
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 this is too technical of a description late-init. Maybe saying something along the lines of it will fill the blanks in spec by defaults from the cloud provider
would be easier to understand what it's for.
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.
} | ||
``` | ||
|
||
In most cases, custom late-initialization configuration will not be necessary. |
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.
Up until this point users still don't know why they might need to customize this behavior. So, I think having an example case at the beginning like if you see that behavior, then you need to customize this by following these steps
.
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.
|
||
// Identifier for this resource is assigned by the provider. In other | ||
// words it is not simply the name of the resource. | ||
r.ExternalName = config.IdentifierFromProvider |
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.
Does it seem like this should be the default? Beyond the description you've provided can you include an example of when this might be used or not used?
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 see you go into more detail in https://github.com/crossplane-contrib/terrajet/pull/130/files#diff-d1f30f3feb80f8b6f5335229713aa3a7ee61093ffe823517e9204422b255251aR15 -- It would help if this step of the guide was part of the other guide.
Signed-off-by: Hasan Turken <turkenh@gmail.com>
Signed-off-by: Hasan Turken <turkenh@gmail.com>
Signed-off-by: Hasan Turken <turkenh@gmail.com>
Signed-off-by: Hasan Turken <turkenh@gmail.com>
Signed-off-by: Hasan Turken <turkenh@gmail.com>
Signed-off-by: Hasan Turken <turkenh@gmail.com>
323226e
to
cf45076
Compare
This should be ready for another pass! |
@sergenyalcin it would be great if could have another look with the latest changes in Terrajet and template repo 🙏 |
Hi @turkenh I tested the guide with the latest changes. During testing, I noticed that crd manifests are generated with I observed that the template repository does not consume the latest version of terrajet. @ulucinar opened a PR to consume the latest version of terrajet: crossplane-contrib/provider-jet-template#6 I created a repository from this PR and noticed a few changes are necessary due to the use of the latest version. I fixed the problems by my last commit. The |
Signed-off-by: Sergen Yalçın <yalcinsergen97@gmail.com>
827ffba
to
458a26e
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.
Thanks @turkenh ! Looks great!
Description of your changes
This PR adds two document:
Fixes #42
I have:
make reviewable
to ensure this PR is ready for review.backport release-x.y
labels to auto-backport this PR if necessary.How has this code been tested
Run the steps end to end and verify it works.