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

external name: use function in the config directly instead of generating the call #124

Merged
merged 3 commits into from
Oct 20, 2021

Conversation

muvaf
Copy link
Member

@muvaf muvaf commented Oct 19, 2021

Description of your changes

We used to print the function call in terraformed.go, but with the recent changes and learnings it seems like calling the function directly on the config object during runtime will enable us to implement more streamlined features. See details in #123

See crossplane-contrib/provider-jet-aws#122 for its application. Once merged, I'll do the same changes in Azure as well.

Fixes #123

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

Manually with AWS VPC.

…ing the call in zz_terraformed.go

Signed-off-by: Muvaffak Onus <me@muvaf.com>
pkg/config/resource.go Outdated Show resolved Hide resolved
Signed-off-by: Muvaffak Onus <me@muvaf.com>
Signed-off-by: Muvaffak Onus <me@muvaf.com>
package config

// Common ExternalName configurations.
var (
Copy link
Member Author

Choose a reason for hiding this comment

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

@turkenh I just added those since they seem to be very common. For the plan of generating all automatically, I think we can use these in the providers, i.e. a for loop setting a new configuration for every resource and maybe it can check existence of name attribute in schema and decides which one to use.

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good, also we could expose this as a provider-level configuration (e.g. default external name config) that could still be overridden by specific resources. For example, for azure, we can just set provider.defaultExternalNameConfig = IdentifierFromProvider and done.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah but if we're going to have that for loop anyway, it could be simpler to set it every time so that we'd worry about only a single field in a single config object instead of checking the default if it doesn't exist.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe we can find a way not to have that loop :)

pkg/config/common.go Show resolved Hide resolved
package config

// Common ExternalName configurations.
var (
Copy link
Member

Choose a reason for hiding this comment

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

Sounds good, also we could expose this as a provider-level configuration (e.g. default external name config) that could still be overridden by specific resources. For example, for azure, we can just set provider.defaultExternalNameConfig = IdentifierFromProvider and done.

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.

Call external name configurator in controller
2 participants