Skip to content

Conversation

@steveej
Copy link
Contributor

@steveej steveej commented Sep 4, 2018

On some systems GOPATH might not be set. The go compiler can be used to emit the default location.

@coreosbot
Copy link

Can one of the admins verify this patch?

@openshift-ci-robot openshift-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Sep 4, 2018
@steveej
Copy link
Contributor Author

steveej commented Sep 4, 2018

/assign @wking

Copy link
Member

@wking wking Sep 4, 2018

Choose a reason for hiding this comment

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

Or it might be in GOBIN. But unfortunately go env GOBIN does not give a sane default when the associated environment variable is unset (at least as of Go 1.10.3). And even without GOBIN, GOPATH could have multiple colon-delimited entries. For something more(ish) reliable, see the stuff starting here. But that seems like a bit much for a HOWTO ;). I'm fine leaving this as it stands, and expecting readers to adjust as needed.

Or maybe we can just set GOBIN for the one-off install?

GOBIN=~/.terraform.d/plugins go get github.com/crawford/terraform-provider-libvirt

That seems to work when I test it locally.

Copy link
Contributor Author

@steveej steveej Sep 4, 2018

Choose a reason for hiding this comment

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

I'm fine leaving this as it stands

You mean without this PR?

Or maybe we can just set GOBIN for the one-off install?

go env GOBIN is empty on my system, so not a valid option.

EDIT1:
I was too quick reading your comment. Installing it that way works here too.

Copy link
Member

@wking wking Sep 4, 2018

Choose a reason for hiding this comment

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

You can drop these two lines with the GOBIN approach. Test locally with:

$ mv ~/.terraform.d /tmp/  # stick your existing directory somewhere else
$ GOBIN=~/.terraform.d/plugins go get github.com/crawford/terraform-provider-libvirt
$ tree ~/.terraform.d  # see that it worked
/home/trking/.terraform.d
└── plugins
    └── terraform-provider-libvirt

1 directory, 1 file
$ rm -rf ~/.terraform.d  # blow away the test
$ mv /tmp/.terraform.d/ ~  # move the original stuff back into place

Copy link
Contributor Author

@steveej steveej Sep 4, 2018

Choose a reason for hiding this comment

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

  1. I missed the duplicate line and 2. was too lazy to test if it would create the directory if not present, thank you! Even better now ;-)

On some systems GOPATH might not be set.
@wking
Copy link
Member

wking commented Sep 4, 2018

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 4, 2018
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: steveeJ, wking

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 4, 2018
@openshift-merge-robot openshift-merge-robot merged commit 36301ff into openshift:master Sep 4, 2018
@steveej steveej deleted the patch-1 branch September 4, 2018 20:05
abhinavdahiya added a commit to abhinavdahiya/installer that referenced this pull request Feb 13, 2019
The `DNS` object [2] was expanded to include the pointers to the public and private hosted zones to allow operators to create DNS records inthe correct zone in PR openshift#202 [4].

The public zone is fetched from the `basedomain` while since installer creates the private zone at later stage, tags are used to specify the private dns zone.

The ID returned by `get-hosted-zone` [1] is of the form `/hostedzone/<id>`. For example,

```console
$ AWS_PROFILE=openshift-dev aws route53 get-hosted-zone --id XXX
{
    "HostedZone": {
        "Id": "/hostedzone/XXX",
...
}
```

This creates the `DNS` [2] object like:
```yaml
apiVersion: config.openshift.io/v1
kind: DNS
metadata:
  name: cluster
spec:
  baseDomain: yyy.openshift.com
  privateZone:
    tags:
      Name: adahiya-1_int
      kubernetes.io/cluster/adahiya-1: owned
      openshiftClusterID: 25375c13-3c0a-4102-a8f9-7ecb60757c62
  publicZone:
    id: "/hostedzone/XXX"
status: {}
```

But you can actually use both `/hostedzone/XXX` or `XXX` to get a zone.

```console
$ AWS_PROFILE=openshift-dev aws route53 get-hosted-zone --id "/hostedzone/XXX"
{
    "HostedZone": {
        "Id": "/hostedzone/XXX",
...
}
```

The change trims the `zoneID` to not include the prefix when creating the `DNS` object as just the ID has cleaner semantics when stored in the object. The terraform-provider-aws also cleans the zone-id for better UX [3] by trimming the prefix `/hostedzone/`.

[1]: https://docs.aws.amazon.com/cli/latest/reference/route53/get-hosted-zone.html#synopsis
[2]: https://github.com/openshift/api/blob/8c839bc7ff62e38ad7656bf920e11e2664a44f6b/config/v1/types_dns.go#L11
[3]: https://github.com/terraform-providers/terraform-provider-aws/blob/75a9ebb7bfc7fd61b4454589155cc6b958ebebe4/aws/resource_aws_route53_zone.go#L461-L464
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants