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

refactor: removing unused variable #488

Merged
merged 8 commits into from
Apr 12, 2023
Merged

refactor: removing unused variable #488

merged 8 commits into from
Apr 12, 2023

Conversation

imprateeksh
Copy link
Member

@imprateeksh imprateeksh commented Apr 5, 2023

Description

Enhancement: To remove the unused optional variable network_connections
Refer issue 485 for details.

Types of changes in this PR

No release required

  • Examples or tests (addition or updates of examples or tests)
  • Documentation update
  • CI-related update (pipeline, etc.)
  • [] Other changes that don't affect Terraform code

Release required

  • Bug fix (patch release (x.x.X): Change that fixes an issue and is compatible with earlier versions)
  • New feature (minor release (x.X.x): Change that adds functionality and is compatible with earlier versions)
  • Breaking change (major release (X.x.x): Change that is likely incompatible with previous versions)
Release notes content

fix: The network_connections option has been removed from the network_acls variable as it was not being used in the code

BREAKING CHANGE: If you are upgrading to this version, and you have the network_connections option in the network_acls variable, it should be removed.


Checklist for reviewers

  • If relevant, a test for the change is included or updated with this PR.
  • If relevant, documentation for the change is included or updated with this PR.

Merge actions for mergers

  • Merge by using "Squash and merge".

  • Use a relevant conventional commit message that is based on the PR contents and any release notes provided by the PR author.

    The commit message determines whether a new version of the module is needed, and if so, which semver increment to use (major, minor, or patch).

@imprateeksh imprateeksh self-assigned this Apr 5, 2023
@imprateeksh imprateeksh changed the title refactor: removing unused variable [DO NOT MERGE] refactor: removing unused variable Apr 5, 2023
@imprateeksh imprateeksh changed the title [DO NOT MERGE] refactor: removing unused variable refactor: removing unused variable Apr 5, 2023
@imprateeksh imprateeksh requested a review from vburckhardt April 5, 2023 18:12
@imprateeksh imprateeksh marked this pull request as ready for review April 5, 2023 18:14
@imprateeksh
Copy link
Member Author

This passed successfully but once integrated latest main changes today, can see some failure in the upgrade test.
It appears that network ACL rule already exists.
image

Re-running first to see if this error still occurs.

@imprateeksh
Copy link
Member Author

Validation succeeded after re-running the jobs.

variables.tf Outdated Show resolved Hide resolved
@ocofaigh
Copy link
Member

ocofaigh commented Apr 7, 2023

@imprateeksh why have you ticked the boxes for Documentation update and Other changes that don't affect Terraform code ? This is a change to a terraform variable. Infact its probably a breaking change which will warrant a major version update. Don't you agree?

Copy link
Contributor

@SirSpidey SirSpidey left a comment

Choose a reason for hiding this comment

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

small suggestion

module-metadata.json Outdated Show resolved Hide resolved
@imprateeksh
Copy link
Member Author

imprateeksh commented Apr 7, 2023

@imprateeksh why have you ticked the boxes for Documentation update and Other changes that don't affect Terraform code ? This is a change to a terraform variable. Infact its probably a breaking change which will warrant a major version update. Don't you agree?

HI @ocofaigh - Documentation update was chosen as there was change in the description of the variable. Indeed it is breaking change and Other changes that don't affect Terraform code is not applicable here.
I have made these corrections. Thank you for pointing out.

@ocofaigh ocofaigh merged commit 276e463 into main Apr 12, 2023
@ocofaigh ocofaigh deleted the 485-cleanup-vars branch April 12, 2023 18:22
@terraform-ibm-modules-ops
Copy link
Contributor

🎉 This PR is included in version 6.0.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants