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

Have option to specify gitops path where to place generated files #61

Merged
merged 1 commit into from
Mar 2, 2021

Conversation

arturo-skydio
Copy link
Contributor

@arturo-skydio arturo-skydio commented Feb 26, 2021

Description

Currently, gitops files get generated under a hardcoded prefix path of cloud, need a way to have a variable input for this

Related Issue

#60

Motivation and Context

For repositories that do not want gitops files generated at cloud path, this change is needed

How Has This Been Tested?

Just started using this and verified that no gitops_path renders gitops files in usual cloud directory, while specifying path correctly changes the prefix path

Types of changes

  • New feature (non-breaking change which adds functionality)

Checklist:

  • I have signed the Adobe Open Source CLA.
  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@arturo-skydio arturo-skydio requested a review from a team as a code owner February 26, 2021 22:10
@michaelschiff
Copy link
Collaborator

@arturo-skydio thank you for the contribution! mind signing the CLA?

@arturo-skydio
Copy link
Contributor Author

@arturo-skydio thank you for the contribution! mind signing the CLA?

@michaelschiff I thought I did,
not sure if its because the change comes from our skydio org, maybe I should just try to open the PR from my gh account?

@arturo-skydio arturo-skydio deleted the cloud-gitops-hardcode-path branch February 26, 2021 22:53
@arturo-skydio arturo-skydio restored the cloud-gitops-hardcode-path branch February 26, 2021 22:53
@arturo-skydio arturo-skydio reopened this Feb 26, 2021
@arturo-skydio
Copy link
Contributor Author

@michaelschiff sorry, was a bit impatient with opening up the other PR, looks like the CLA is green now, thanks

Copy link
Member

@kzadorozhny kzadorozhny left a comment

Choose a reason for hiding this comment

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

Looks good overall. No issue with the change. But there are couple issues that need to be addressed before we can merge:

  • You might want to add another test case into the e2e_test. Right now we rely on cloud directory being default. For example you can add another k8s_deploy target alongside release and canary targets and update the script. Don't forget to update the .gitignore file too.
  • cloud also is hardcoded in couple places ( 1, 2 ) in the GitOps PRs tool.

@arturo-skydio
Copy link
Contributor Author

Looks good overall. No issue with the change. But there are couple issues that need to be addressed before we can merge:

  • You might want to add another test case into the e2e_test. Right now we rely on cloud directory being default. For example you can add another k8s_deploy target alongside release and canary targets and update the script. Don't forget to update the .gitignore file too.
  • cloud also is hardcoded in couple places ( 1, 2 ) in the GitOps PRs tool.

Sounds good, thanks for taking a look, I'll add the test

@arturo-skydio arturo-skydio force-pushed the cloud-gitops-hardcode-path branch 5 times, most recently from d75ee57 to 3f96d03 Compare March 2, 2021 16:26
@arturo-skydio
Copy link
Contributor Author

@kzadorozhny Added a test for gitops_path and made sure to remove hardcoding in the Gitops Prs tool, let me know if all look good, thanks again

Copy link
Member

@kzadorozhny kzadorozhny left a comment

Choose a reason for hiding this comment

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

Overall LGTM. Please address one comment and I'll merge it.

@arturo-skydio arturo-skydio force-pushed the cloud-gitops-hardcode-path branch from 3f96d03 to 49505be Compare March 2, 2021 18:11
@kzadorozhny kzadorozhny merged commit 3f0bfcb into adobe:master Mar 2, 2021
@arturo-skydio arturo-skydio deleted the cloud-gitops-hardcode-path branch March 2, 2021 22:14
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.

3 participants