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

Configure External Name #6

Merged
merged 6 commits into from
Nov 19, 2021
Merged

Conversation

turkenh
Copy link
Collaborator

@turkenh turkenh commented Nov 19, 2021

Description of your changes

This PR configures external-name properly with latest Terrajet config options.

I have:

  • Read and followed Crossplane's contribution process.
  • Run make reviewable test to ensure this PR is ready for review.

How has this code been tested

Deploy example compute instance.

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 turkenh changed the title Configure Configure External Name Nov 19, 2021
@turkenh turkenh requested review from muvaf and ulucinar November 19, 2021 06:47

func externalNameOverride() tjconfig.ResourceOption {
return func(resource *tjconfig.Resource) {
resource.ExternalName.GetExternalNameFn = common.GetNameFromFullyQualifiedID
Copy link
Member

@muvaf muvaf Nov 19, 2021

Choose a reason for hiding this comment

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

GetExternalNameFn should be compatible with GetIDFn. Right now, it would return the last part of the ID as external name and then when it constructs the ID, it will copy that external name. My suggestion would be to use a provider-wide default for the whole resource.ExternalName configuration, instead of only resource.ExternalName.GetExternalNameFn

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'll work on a provider-wide default for external name in a follow-up PR.
Reverted defaulting for now and moved to individual resource configs.

Copy link
Member

Choose a reason for hiding this comment

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

I meant something like common.IdentifierFromProvider that has all sub-configs compatible for each other.

config/overrides.go Show resolved Hide resolved
Signed-off-by: Hasan Turken <turkenh@gmail.com>
@turkenh turkenh requested a review from muvaf November 19, 2021 09:44
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 !

@turkenh turkenh merged commit 267a966 into crossplane-contrib: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.

2 participants