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!: Namegenerator functions refactor #12

Merged
merged 7 commits into from
Sep 25, 2023

Conversation

horiagunica
Copy link
Contributor

Description

Refactor Name Generator function into a single function. Also introduce obtaining GCP project ID based on repo secrets environment variables.

This PR closes #9 .

Motivation and Context

Code refactoring.

How Has This Been Tested?

Tested using separate test repo : https://github.com/PaloAltoNetworks/test-gcp-ci-workflows/ .

Tested by pointing the GO module to the specific branch .

Types of changes

  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes if appropriate.
  • All new and existing tests passed.

@horiagunica horiagunica requested review from a team as code owners September 19, 2023 11:23
@welcome-to-palo-alto-networks

🎉 Thanks for opening this pull request! We really appreciate contributors like you! 🙌

@horiagunica horiagunica changed the title Namegenerator functions refactor refacto: Namegenerator functions refactor Sep 19, 2023
@horiagunica horiagunica changed the title refacto: Namegenerator functions refactor refactor: Namegenerator functions refactor Sep 19, 2023
Copy link
Contributor

@sebastianczech sebastianczech left a comment

Choose a reason for hiding this comment

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

Great job, thank you for refactoring code and doing 1 function (instead multiple ones per cloud) 👍

Besides below comments, I have also 1 more question - what is the reason to pass PROJECT_ID via Terratest skeleton , do not change anything in that value and return it in struct TerraformVarsInfo ? Maybe we could directly get project ID in tests in repository terraform-google-vmseries-modules, without passing that value into skeleton and reading it from outputs ?

pkg/testskeleton/testskeleton.go Outdated Show resolved Hide resolved
pkg/testskeleton/testskeleton.go Outdated Show resolved Hide resolved
Copy link
Contributor

@sebastianczech sebastianczech left a comment

Choose a reason for hiding this comment

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

great job 👍
please change PR title from refactor: into refactor!: as we have breaking change.

@horiagunica horiagunica changed the title refactor: Namegenerator functions refactor refactor!: Namegenerator functions refactor Sep 21, 2023
Copy link
Contributor

@sebastianczech sebastianczech left a comment

Choose a reason for hiding this comment

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

👍

Copy link
Contributor

@FoSix FoSix left a comment

Choose a reason for hiding this comment

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

👍🏻

@horiagunica horiagunica merged commit 8accb51 into main Sep 25, 2023
@horiagunica horiagunica deleted the namegenerator-functions-refactor branch September 25, 2023 07:47
@welcome-to-palo-alto-networks

🎉 Congrats on getting your first pull request merged! We here at Palo Alto Networks are so grateful! ❤️

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.

Refactor NameGenerator functions
3 participants