[go: nahoru, domu]

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

fix: PR930 ArgoCD pipeline set the namespace based on user input #1081

Closed
wants to merge 1 commit into from

Conversation

zyy98
Copy link
@zyy98 zyy98 commented Aug 26, 2023

Link to bug: #930

This PR gets the namespace based on user input.

@github-actions
Copy link

Thanks for making a pull request! 😃
One of the maintainers will review and advise on the next steps.

@github-actions github-actions bot added the fix label Aug 26, 2023
Signed-off-by: yzhang2480 <yzhang2480@bloomberg.net>
@codecov
Copy link
codecov bot commented Aug 26, 2023

Codecov Report

Patch coverage has no change and project coverage change: +0.01% 🎉

Comparison is base (29c061c) 14.86% compared to head (ff39df3) 14.87%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1081      +/-   ##
==========================================
+ Coverage   14.86%   14.87%   +0.01%     
==========================================
  Files          86       86              
  Lines        8181     8174       -7     
==========================================
  Hits         1216     1216              
+ Misses       6649     6642       -7     
  Partials      316      316              
Files Changed Coverage Δ
...former/kubernetes/apiresource/argocdapplication.go 0.00% <0.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor
@Akash-Nayak Akash-Nayak left a comment

Choose a reason for hiding this comment

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

Thanks for your first PR contribution to Move2Kube! A few suggestions..

@@ -155,6 +155,8 @@ const (
ConfigCICDTektonGitRepoBasicAuthSecretNameKey = ConfigCICDTektonKey + d + "gitrepobasicauthsecret"
// ConfigCICDTektonRegistryPushSecretNameKey is for Tekton push image to registry credentials
ConfigCICDTektonRegistryPushSecretNameKey = ConfigCICDTektonKey + d + "registrypushsecret"
//ConfigCICDArgoDestinationNameSpace gives the user input namespace destination
ConfigCICDArgoDestinationNameSpace = "destnamespace"
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you change the variable name from ConfigCICDArgoDestinationNameSpace to ConfigCICDArgoCDDestinationNameSpaceKey?

Also, can you add a new constantConfigCICDArgoCDKey = ConfigCICDKey + d + "argocd" (like we have for Tekton here), and change the value of ConfigCICDArgoCDDestinationNameSpaceKey to ConfigCICDArgoCDKey + d + "destnamespace"

@@ -70,6 +71,16 @@ func (*ArgoCDApplication) createNewResource(irApplication irtypes.Application, t
clusterServer = deployToSameCluster
}
appGVK := v1alpha1.ApplicationSchemaGroupVersionKind
destNamespace := qaengine.FetchStringAnswer(
common.ConfigCICDArgoDestinationNameSpace,
"Enter the destination namespace for argo cd pipeline",
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we change the string to "Enter the destination namespace for the Argo CD pipeline"

destNamespace := qaengine.FetchStringAnswer(
common.ConfigCICDArgoDestinationNameSpace,
"Enter the destination namespace for argo cd pipeline",
[]string{},
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of providing empty context []string{}, can we provide a context something like []string{If Argo CD pipeline is not relevant to you, then leave empty to use the default value for it.}

@Akash-Nayak
Copy link
Contributor

Can you also please rebase the branch (https://github.com/konveyor/move2kube/blob/main/contributing.md#pull-request-process), as it is out-of-date with the base branch?

@HarikrishnanBalagopal
Copy link
Contributor

Fixed by #1069

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.

None yet

3 participants