Skip to content
This repository has been archived by the owner on Dec 15, 2022. It is now read-only.

Generating a Provider guide #130

Merged
merged 7 commits into from
Nov 19, 2021
Merged

Conversation

turkenh
Copy link
Member

@turkenh turkenh commented Nov 1, 2021

Description of your changes

This PR adds two document:

  • Generating a Provider using Terrajet
  • Configuring a Resource

Fixes #42

I have:

  • Read and followed Crossplane's contribution process.
  • Run make reviewable to ensure this PR is ready for review.
  • Added 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.

@turkenh turkenh force-pushed the generating-provider-guide branch 2 times, most recently from 0461986 to 5dcead1 Compare November 1, 2021 20:30
@turkenh turkenh changed the title [WIP] Generating provider guide [WIP] Guide - Generating a provider Nov 2, 2021
@turkenh turkenh force-pushed the generating-provider-guide branch 8 times, most recently from b8c4612 to 142cc63 Compare November 5, 2021 15:08
@turkenh turkenh changed the title [WIP] Guide - Generating a provider Guide - Generating a provider Nov 5, 2021
@turkenh turkenh changed the title Guide - Generating a provider Guide - Generating a Provider Nov 5, 2021
@turkenh turkenh changed the title Guide - Generating a Provider Generating a Provider guide Nov 5, 2021
@turkenh turkenh marked this pull request as ready for review November 5, 2021 15:10
@turkenh turkenh force-pushed the generating-provider-guide branch 2 times, most recently from 5efdb6e to 323226e Compare November 5, 2021 17:36
@sergenyalcin
Copy link
Member

sergenyalcin commented Nov 6, 2021

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.

@turkenh
Copy link
Member Author

turkenh commented Nov 8, 2021

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
Copy link
Member

@displague displague Nov 12, 2021

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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
```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.

Copy link
Member Author

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.

Comment on lines +145 to +146
"github_repository$",
"github_branch$",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"github_repository$",
"github_branch$",
"github_repository",
"github_branch",

Would it be easier to understand if we targeted single resources here instead of having prefixes?

Copy link
Member

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.

Copy link
Member

@displague displague Nov 17, 2021

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nm - I found WithSkipList

Copy link
Member Author

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

docs/generating-a-provider.md Outdated Show resolved Hide resolved
}))
```

7. Finally, we would need to add some custom configurations for these two
Copy link
Member

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.

Copy link
Member

@displague displague Nov 17, 2021

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.

Copy link
Member Author

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
Copy link
Member

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

Copy link
Member Author

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 Show resolved Hide resolved
Comment on lines 252 to 253
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
Copy link
Member

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

Copy link
Member Author

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?

Copy link
Member Author

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`.
Copy link
Member

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ulucinar what do you think?
In any case, I would prefer to postpone rewdoring this to #153

}
```

In most cases, custom late-initialization configuration will not be necessary.
Copy link
Member

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Crosslinking to #153
//cc @ulucinar

docs/generating-a-provider.md Show resolved Hide resolved

// 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
Copy link
Member

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?

Copy link
Member

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>
@turkenh
Copy link
Member Author

turkenh commented Nov 19, 2021

This should be ready for another pass!

@turkenh
Copy link
Member Author

turkenh commented Nov 19, 2021

@sergenyalcin it would be great if could have another look with the latest changes in Terrajet and template repo 🙏

@sergenyalcin
Copy link
Member

@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 tf apigroup names. However in the examples, jet name was used as apigroup.

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 r.Group field in the config/repository/config.go and config/branch/config.go was converted to r.ShortGroup.

Signed-off-by: Sergen Yalçın <yalcinsergen97@gmail.com>
Copy link
Member

@muvaf muvaf left a 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!

@muvaf muvaf merged commit d9b1c93 into crossplane:main Nov 19, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Guide to generate a new TF provider
5 participants