Skip to content
This repository has been archived by the owner on Feb 23, 2024. It is now read-only.

Add first-level IP address variables #18

Merged
merged 1 commit into from
Sep 6, 2017

Conversation

bzub
Copy link
Contributor

@bzub bzub commented Sep 2, 2017

  • access_public_ipv6
  • access_public_ipv4
  • access_private_ipv4

These are copied from the "network" list of maps for convenience.

Fixes #11

@bzub
Copy link
Contributor Author

bzub commented Sep 2, 2017

Ready for review and acceptance tests!

"access_public_ipv6": &schema.Schema{
Type: schema.TypeString,
Computed: true,
ForceNew: true,
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the ForceNew necessary in computed fields? Maybe @radeksimko could clarify.

Copy link
Contributor

@radeksimko radeksimko Sep 5, 2017

Choose a reason for hiding this comment

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

Fields can be both Computed and Optional in which case they may also be ForceNew, but a Computed-only field can't really be ForceNew, because such field never occurs in the config, so there's nothing to trigger the recreation process. API changing computed values isn't interpreted as change, it's just simply written to state and updated wherever it's referenced.

Thanks for pointing this out - I'll see if we can add this to our schema validation logic
https://github.com/hashicorp/terraform/blob/cbb512d374a80d9d2c7cd49ca0644c576ab2335f/helper/schema/schema.go#L548

if ip.AddressFamily == 4 && ip.Public == true {
host = ip.Address
ipv4SubnetSize = ip.CIDR
if ip.AddressFamily == 4 {
Copy link
Contributor

@t0mk t0mk Sep 5, 2017

Choose a reason for hiding this comment

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

The default public IPv4 has Management: True, and it should be checked to get the access_public_ipv4.

However, the management field is not exported by packngo (my fault). Can we please hold this PR until I add Management to Packngo.

@t0mk
Copy link
Contributor

t0mk commented Sep 5, 2017

I run the basic acceptance test, and beside my comments in the review it it's alright.

@t0mk
Copy link
Contributor

t0mk commented Sep 5, 2017

Blocked by equinixmetal-archive/packngo#40

@bzub please review the linekd packngo PR if you have time.

- access_public_ipv6
- access_public_ipv4
- access_private_ipv4

These are copied from the "network" list of maps for convenience.

Add test step to validate access IP address strings

Document new first-level IP address device fields

Don't use ForceNew on management IPs
If these IPs change, something must be wrong with the API or this
plugin. So to be safe, don't force a recreation of the device/resource.

Use management designation to determine access IPs
@bzub bzub force-pushed the 11-access-ip-vars branch from 6146b61 to cedf707 Compare September 6, 2017 15:10
@bzub
Copy link
Contributor Author

bzub commented Sep 6, 2017

Please take another look, thanks!

@t0mk t0mk merged commit 591d37c into packethost:master Sep 6, 2017
@bzub bzub deleted the 11-access-ip-vars branch September 18, 2017 00:43
displague pushed a commit to displague/terraform-provider-packet that referenced this pull request Mar 5, 2021
Tweak the issue template to match the repository
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.

3 participants