Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable VPC module to accept subnetwork_name input variable #285

Merged
merged 5 commits into from
May 11, 2022
Merged

Enable VPC module to accept subnetwork_name input variable #285

merged 5 commits into from
May 11, 2022

Conversation

tpdownes
Copy link
Member

@tpdownes tpdownes commented May 9, 2022

  • Expose subnetwork_name as a top-level variable to the VPC module to allow multi-group blueprints to succeed when network_name and subnetwork_name are set as global variables. In particular, this allows the pre-existing-vpc module in group N to detect a VPC created by the vpc module in group M, where M < N.
  • Re-work primary_subnetwork as an input variable that defaults to null.
    If no value is supplied, then a default is constructed in a local
    variable. If a value is supplied, then it is taken explicitly.
  • Construct local block from new input variables subnetwork_name and
    subnetwork_size. The former aligns with Toolkit naming conventions and
    simplifies multi-group blueprints when network_name and subnetwork_name
    are set as global variables. In particular, this allows the
    pre-existing-vpc module in group N to detect a VPC created by the vpc
    module in group M, where M < N.
  • Adopt feature throughout internal tests and user-facing examples

Submission Checklist

  • Have you installed and run this change against pre-commit? pre-commit install
  • Are all tests passing? make tests
  • If applicable, have you written additional unit tests to cover this
    change?
  • Is unit test coverage still above 80%?
  • Have you updated any application documentation such as READMEs and user
    guides?
  • Have you followed the guidelines in our Contributing document?

Copy link
Member

@cboneti cboneti left a comment

Choose a reason for hiding this comment

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

I have a usability suggestion.

modules/network/vpc/variables.tf Outdated Show resolved Hide resolved
modules/network/vpc/main.tf Outdated Show resolved Hide resolved
@cboneti cboneti assigned tpdownes and unassigned cboneti May 10, 2022
@tpdownes tpdownes assigned cboneti and unassigned tpdownes May 10, 2022
@tpdownes
Copy link
Member Author

I have made modification based on out-of-band conversation to re-work var.primary_subnetwork as an explicit value which defaults to null but, if supplied, will be passed explicitly.

@tpdownes tpdownes assigned tpdownes and unassigned cboneti May 10, 2022
@tpdownes
Copy link
Member Author

Build tests failed due to hitting an internal Filestore quota. Re-running them. GitHub should update once they complete.

@tpdownes tpdownes assigned cboneti and unassigned tpdownes May 10, 2022
Copy link
Member

@cboneti cboneti left a comment

Choose a reason for hiding this comment

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

a few more comments.

Primary (default) subnetwork in which to create resources. If null, a default value will be constructed.

subnet_name (string, required, Name of subnet; will be replaced by var.subnetwork_name or its default value)
subnet_region (string, requiered, will be replaced by var.region)
Copy link
Member

Choose a reason for hiding this comment

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

typo: requiered. :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.

modules/network/vpc/variables.tf Show resolved Hide resolved
purpose (string, optional, related to Load Balancing)
role (string, optional, related to Load Balancing)
subnet_name (string, required, Name of subnet; will be replaced by var.subnetwork_name or its default value)
subnet_region (string, requiered, will be replaced by var.region)
Copy link
Member

Choose a reason for hiding this comment

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

same required typo here

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed

modules/network/vpc/variables.tf Show resolved Hide resolved
@cboneti cboneti assigned tpdownes and unassigned cboneti May 10, 2022
* Re-work primary_subnetwork as an input variable that defaults to null.
  If no value is supplied, then a default is constructed in a local
  variable. If a value is supplied, then it is taken explicitly.
* Construct local block from new input variables subnetwork_name and
  subnetwork_size. The former aligns with Toolkit naming conventions and
  simplifies multi-group blueprints when network_name and subnetwork_name
  are set as global variables. In particular, this allows the
  pre-existing-vpc module in group N to detect a VPC created by the vpc
  module in group M, where M < N.
* Adopt new feature in VPC module to allow explicit specification of
  both network_name and subnetwork_name; current image building example
  works because Packer template defaults to same subnetwork_name as VPC
  module
* The Packer template was previously designed to default to the same
  subnetwork_name as is created by the VPC module. This behavior was
  intentional to allow for more automatic harmony when a user creates
  a VPC without specifying a value.
* This change intentionally requires the user to specify
  subnetwork_name. It is anticipated that the most common use case
  will be to specify subnetwork_name globally.
* Current solution relies on alignment of globally-set subnetwork_name
  with value created by VPC module. Updates to the VPC module no longer
  require this, so adopt a value that is more arbitrary.
@tpdownes tpdownes assigned cboneti and unassigned tpdownes May 11, 2022
@cboneti cboneti assigned tpdownes and unassigned cboneti May 11, 2022
@tpdownes tpdownes merged commit b06a0c7 into GoogleCloudPlatform:develop May 11, 2022
@tpdownes tpdownes deleted the feat_vpc_accept_subnetwork branch May 11, 2022 15:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants