astro-cli should prompt the user for namespace name input when pre-deployment validation is configured #487

ajayy004 · 2022-01-25T19:18:14Z

Description

based on pre-deployment validation config, user should be prompted for k8 namespace name to be enter instead of select from available k8 namespaces

🎟 Issue(s)

Related astronomer/issues#4132

🧪 Functional Testing

List the functional testing steps to confirm this feature or fix.

📸 Screenshots

Add screenshots to illustrate the validity of these changes.

📋 Checklist

Rebased from the main (or release if patching) branch (before testing)
Ran make test before taking out of draft
Added/updated applicable tests
Tested against Houston-API and Astronomer (if necessary).
Communicated to/tagged owners of respective clients potentially impacted by these changes.
Updated any related documentation

…r k8 namespace name to be enter instead of select from available k8 namespaces

ajayy004 · 2022-01-25T19:19:23Z

deployment/deployment.go

+func getDeploymentNamespaceName() (string, error) {
+	namespaceName := input.Text("\nKubernetes Namespace Name: ")
+	noSpaceString := strings.ReplaceAll(namespaceName, " ", "")
+	if noSpaceString == "" {


Does it make sense to do empty string validation?

It does make sense to me to validate the input, but it would also be decent idea to strip unwanted spaces from the input text entered by the user, i.e.

namespaceName = strings.ReplaceAll(namespace, " ", "") if namespaceName == "" { return "", ErrKubernetesNamespaceNotSpecified } return namespaceName, nil

codecov · 2022-01-25T19:21:02Z

Codecov Report

Merging #487 (19a80f5) into main (01f98c5) will increase coverage by 0.14%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #487      +/-   ##
==========================================
+ Coverage   66.30%   66.45%   +0.14%     
==========================================
  Files          42       42              
  Lines        3642     3658      +16     
==========================================
+ Hits         2415     2431      +16     
  Misses       1054     1054              
  Partials      173      173

Impacted Files	Coverage Δ
houston/types.go	`55.00% <ø> (ø)`
deployment/deployment.go	`86.14% <100.00%> (+0.62%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 01f98c5...19a80f5. Read the comment docs.

…n webhook)

deployment/deployment.go

neel-astro

Great work 👍 , have added few minor suggestions

neel-astro · 2022-01-27T06:08:14Z

deployment/deployment.go

@@ -133,6 +134,14 @@ func CheckPreCreateNamespaceDeployment(client *houston.Client) bool {
 	return appConfig.Flags.ManualNamespaceNames
 }

+func CheckNamespaceFreeformEntryDeployment(client *houston.Client) bool {


Nit: CheckNamespaceFreeformEntryDeployment => checkNamespaceFreeformEntryDeployment

If the scope of this function is just limited to this package, then we can keep this function private (any field, method or function starting with uppercase letter will be a public function, and if it is starting with lowercase letter then it will be a private function).

Thanks, I think it will make more sense to make it private since I don't see a use case for now where we can call this from outside.

neel-astro · 2022-01-27T06:29:54Z

deployment/deployment.go

+func getDeploymentNamespaceName() (string, error) {
+	namespaceName := input.Text("\nKubernetes Namespace Name: ")
+	noSpaceString := strings.ReplaceAll(namespaceName, " ", "")
+	if noSpaceString == "" {


It does make sense to me to validate the input, but it would also be decent idea to strip unwanted spaces from the input text entered by the user, i.e.

namespaceName = strings.ReplaceAll(namespace, " ", "") if namespaceName == "" { return "", ErrKubernetesNamespaceNotSpecified } return namespaceName, nil

neel-astro · 2022-01-27T06:39:00Z

deployment/deployment.go

@@ -146,14 +155,22 @@ func CheckTriggererEnabled(client *houston.Client) bool {
 func Create(label, ws, releaseName, cloudRole, executor, airflowVersion, dagDeploymentType, nfsLocation, gitRepoURL, gitRevision, gitBranchName, gitDAGDir, sshKey, knownHosts string, gitSyncInterval, triggererReplicas int, client *houston.Client, out io.Writer) error {
 	vars := map[string]interface{}{"label": label, "workspaceId": ws, "executor": executor, "cloudRole": cloudRole}

-	if CheckPreCreateNamespaceDeployment(client) {
+	if CheckPreCreateNamespaceDeployment(client) && !CheckNamespaceFreeformEntryDeployment(client) {


nit: we can get rid of individual function call to get a field value from appConfig, but rather have a single call to Houston to get all the appConfig and then directly picking the individual field throughout the Create method. This will save multiple API calls to Houston to fetch the same appConfig data.

appConfig, err := AppConfig(client) if err != nil { fmt.Println("Error fetching feature flags from Houston: %s", err.Error()) } if appConfig.Flags.ManualNamespaceNames && !appConfig.Flags.NamespaceFreeformEntry { namespace, err := getDeploymentSelectionNamespaces(client, out) if err != nil { return err } vars["namespace"] = namespace } if appConfig.Flags.ManualNamespaceNames && appConfig.Flags.NamespaceFreeformEntry { namespace, err := getDeploymentNamespaceName() if err != nil { return err } vars["namespace"] = namespace } if releaseName != "" && appConfig.ManualReleaseNames { vars["releaseName"] = releaseName } ....... if appConfig.Flags.TriggererEnabled { vars["triggererReplicas"] = triggererReplicas }

Yes I think it does make sense to do.

ajayy004 · 2022-01-27T14:51:15Z

@neel-astro Will it be fine If I open another PR with the suggested changes?

neel-astro · 2022-01-27T15:41:30Z

@neel-astro Will it be fine If I open another PR with the suggested changes?

yeah sure 👍, you can reference this issue if you are consolidating all Houston API calls in Create method.

…ployment validation is configured (#487) * based on pre-deployment validation config, user should be prompted for k8 namespace name to be enter instead of select from available k8 namespaces * added test cases for NamespaceFreeformEntry (Pre-Deployment validation webhook) * fixed broken test cases for namespaceFreeformEntry * fixed broken test cases * updated ErrKubernetesNamespaceNotSpecified message changes create client interface and implementation for houston methods fix linter, rename methods

…ion for houston methods) (#490) * create client interface and implementation for houston methods * update controller packages to use new houston client * update cmds and main to use new houston client * change houston variable visiblity * fix linter, rename methods * moved away from pkg/errors (#484) * astro-cli should prompt the user for namespace name input when pre-deployment validation is configured (#487) * based on pre-deployment validation config, user should be prompted for k8 namespace name to be enter instead of select from available k8 namespaces * added test cases for NamespaceFreeformEntry (Pre-Deployment validation webhook) * fixed broken test cases for namespaceFreeformEntry * fixed broken test cases * updated ErrKubernetesNamespaceNotSpecified message changes create client interface and implementation for houston methods fix linter, rename methods * fix airflow upgrade * fix test and add mocks for houston client w/ mockery * fix houston request errors add proper json tags * rename methods according to code review * fix new log_tests, use renamed houston newClient method * deploy test: add assertions on the houston mock client * move houston mocks to houston directory * add tests for houston package Co-authored-by: Neel Dalsania <92356010+neel-astro@users.noreply.github.com> Co-authored-by: Aj <ajayy101@gmail.com>

…ployment validation is configured (#487) * based on pre-deployment validation config, user should be prompted for k8 namespace name to be enter instead of select from available k8 namespaces * added test cases for NamespaceFreeformEntry (Pre-Deployment validation webhook) * fixed broken test cases for namespaceFreeformEntry * fixed broken test cases * updated ErrKubernetesNamespaceNotSpecified message changes create client interface and implementation for houston methods fix linter, rename methods

* create client interface and implementation for houston methods * change houston variable visiblity * fix linter, rename methods * moved away from pkg/errors (#484) * astro-cli should prompt the user for namespace name input when pre-deployment validation is configured (#487) * based on pre-deployment validation config, user should be prompted for k8 namespace name to be enter instead of select from available k8 namespaces * added test cases for NamespaceFreeformEntry (Pre-Deployment validation webhook) * fixed broken test cases for namespaceFreeformEntry * fixed broken test cases * updated ErrKubernetesNamespaceNotSpecified message changes create client interface and implementation for houston methods fix linter, rename methods * fix test and add mocks for houston client w/ mockery * rename methods according to code review * move houston mocks to houston directory * refactor tests to use houston client mock * rename mocks package to use houston_mocks Co-authored-by: Neel Dalsania <92356010+neel-astro@users.noreply.github.com> Co-authored-by: Aj <ajayy101@gmail.com>

based on pre-deployment validation config, user should be prompted fo…

b083962

…r k8 namespace name to be enter instead of select from available k8 namespaces

ajayy004 commented Jan 25, 2022

View reviewed changes

ajayy004 added 3 commits January 25, 2022 15:45

added test cases for NamespaceFreeformEntry (Pre-Deployment validatio…

f382481

…n webhook)

fixed broken test cases for namespaceFreeformEntry

26841d9

fixed broken test cases

7ff8411

ajayy004 marked this pull request as ready for review January 26, 2022 14:08

ajayy004 requested review from aliotta, andriisoldatenko, bote795 and neel-astro as code owners January 26, 2022 14:08

rdodev reviewed Jan 26, 2022

View reviewed changes

deployment/deployment.go Outdated Show resolved Hide resolved

updated ErrKubernetesNamespaceNotSpecified message changes

2e3d444

neel-astro approved these changes Jan 27, 2022

View reviewed changes

resolved conflicts

19a80f5

ajayy004 requested a review from neel-astro January 27, 2022 15:43

neel-astro approved these changes Jan 27, 2022

View reviewed changes

bote795 approved these changes Jan 27, 2022

View reviewed changes

andriisoldatenko approved these changes Jan 27, 2022

View reviewed changes

andriisoldatenko merged commit e65e362 into main Jan 27, 2022

ajayy004 deleted the feature/4132 branch February 1, 2022 16:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

astro-cli should prompt the user for namespace name input when pre-deployment validation is configured #487

astro-cli should prompt the user for namespace name input when pre-deployment validation is configured #487

ajayy004 commented Jan 25, 2022 •

edited

ajayy004 Jan 25, 2022

neel-astro Jan 27, 2022 •

edited

codecov bot commented Jan 25, 2022 •

edited

neel-astro left a comment

neel-astro Jan 27, 2022

ajayy004 Jan 27, 2022

neel-astro Jan 27, 2022 •

edited

neel-astro Jan 27, 2022

ajayy004 Jan 27, 2022

ajayy004 commented Jan 27, 2022

neel-astro commented Jan 27, 2022

astro-cli should prompt the user for namespace name input when pre-deployment validation is configured #487

astro-cli should prompt the user for namespace name input when pre-deployment validation is configured #487

Conversation

ajayy004 commented Jan 25, 2022 • edited

Description

🎟 Issue(s)

🧪 Functional Testing

📸 Screenshots

📋 Checklist

ajayy004 Jan 25, 2022

Choose a reason for hiding this comment

neel-astro Jan 27, 2022 • edited

Choose a reason for hiding this comment

codecov bot commented Jan 25, 2022 • edited

Codecov Report

neel-astro left a comment

Choose a reason for hiding this comment

neel-astro Jan 27, 2022

Choose a reason for hiding this comment

ajayy004 Jan 27, 2022

Choose a reason for hiding this comment

neel-astro Jan 27, 2022 • edited

Choose a reason for hiding this comment

neel-astro Jan 27, 2022

Choose a reason for hiding this comment

ajayy004 Jan 27, 2022

Choose a reason for hiding this comment

ajayy004 commented Jan 27, 2022

neel-astro commented Jan 27, 2022

ajayy004 commented Jan 25, 2022 •

edited

neel-astro Jan 27, 2022 •

edited

codecov bot commented Jan 25, 2022 •

edited

neel-astro Jan 27, 2022 •

edited