-
Notifications
You must be signed in to change notification settings - Fork 118
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
Conversation
Thanks for making a pull request! 😃 |
Signed-off-by: yzhang2480 <[email protected]>
Codecov ReportPatch coverage has no change and project coverage change:
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
☔ View full report in Codecov by Sentry. |
There was a problem hiding this 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" |
There was a problem hiding this comment.
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", |
There was a problem hiding this comment.
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{}, |
There was a problem hiding this comment.
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.}
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? |
Fixed by #1069 |
Link to bug: #930
This PR gets the namespace based on user input.