Verify Kubernetes context before install #1739

jmgrady · 2022-09-28T14:12:31Z

Update the setup_cluster.py and setup_combine.py scripts to prompt the user to verify the Kubernetes context before the setup tasks are run.

If there is more than one context available, then the script will list the selected context and ask the user to verify the context or select another.

Note that there should be no prompting when run in the CI/CD pipeline.

This change is

codecov-commenter · 2022-09-28T14:14:46Z

Codecov Report

Base: 52.05% // Head: 52.19% // Increases project coverage by +0.14% 🎉

Coverage data is based on head (2f0fc26) compared to base (fc22049).
Patch has no changes to coverable lines.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1739      +/-   ##
==========================================
+ Coverage   52.05%   52.19%   +0.14%     
==========================================
  Files         274      274              
  Lines        8481     8481              
  Branches      616      616              
==========================================
+ Hits         4415     4427      +12     
+ Misses       3574     3562      -12     
  Partials      492      492

Flag	Coverage Δ
backend	`80.44% <ø> (+0.34%)`	⬆️
frontend	`32.66% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
Backend/Helper/DuplicateFinder.cs	`94.38% <0.00%> (+6.74%)`	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

imnasnainaec

Reviewed 4 of 5 files at r1, 5 of 7 files at r2, all commit messages.
Reviewable status: 9 of 11 files reviewed, 2 unresolved discussions (waiting on @jmgrady)

deploy/scripts/aws_env.py line 66 at r2 (raw file):

        os.environ["AWS_SECRET_ACCESS_KEY"] = get_profile_var(aws_profile, "aws_secret_access_key")
        region = get_profile_var(aws_profile, "region")
        os.environ["AWS_DEFAULT_REGION"] = region

Looks like defining region creates an unnecessary extra line.

Code quote:

        region = get_profile_var(aws_profile, "region")
        os.environ["AWS_DEFAULT_REGION"] = region

deploy/scripts/kube_env.py line 80 at r2 (raw file):

    Add commandline arguments that are shared between scripts calling helm.

    Sets up '--verbose' as the equivalent of '--debug'.

This vestigial --verbose reference can be removed.

Code quote:

 Sets up '--verbose' as the equivalent of '--debug'.

Make LiftEntry ids unique.

jmgrady

Reviewable status: 8 of 12 files reviewed, all discussions resolved (waiting on @imnasnainaec and @jmgrady)

deploy/scripts/aws_env.py line 66 at r2 (raw file):

Previously, imnasnainaec (D. Ror.) wrote…

Looks like defining region creates an unnecessary extra line.

Done.

deploy/scripts/kube_env.py line 80 at r2 (raw file):

Previously, imnasnainaec (D. Ror.) wrote…

This vestigial --verbose reference can be removed.

Done.

imnasnainaec

Reviewed 2 of 3 files at r4, all commit messages.
Reviewable status: 10 of 12 files reviewed, 2 unresolved discussions (waiting on @jmgrady)

deploy/scripts/kube_env.py line 77 at r4 (raw file):

def add_kube_opts(parser: argparse.ArgumentParser) -> None:
    """Add commandline arguments for KUbernetes tools."""

Capital U in KUbern...?

deploy/scripts/utils.py line 70 at r4 (raw file):

    or has the curr_selection as its only member.
    """
    if len(options) > 1:

If curr_selection is None and len(options)=1, the present algorithm returns None instead of the single entry in options. Is that okay?

jmgrady

Reviewable status: 10 of 12 files reviewed, 1 unresolved discussion (waiting on @imnasnainaec and @jmgrady)

deploy/scripts/kube_env.py line 77 at r4 (raw file):

Previously, imnasnainaec (D. Ror.) wrote…

Capital U in KUbern...?

Done.

deploy/scripts/utils.py line 70 at r4 (raw file):

Previously, imnasnainaec (D. Ror.) wrote…

If curr_selection is None and len(options)=1, the present algorithm returns None instead of the single entry in options. Is that okay?

No, it is not (and it doesn't match the description!

I have added a test for the case you mentioned at the beginning of the function:

    if len(options) == 1 and curr_selection is not None and curr_selection == options[0]:
       return curr_selection

If this condition is not met, it will prompt the user if len(options) >= 1 instead of strictly > so if the curr_selection is None the user will be prompted to select the option. I don't want to blindly take the single option because if it is not the correct one, The Combine could be installed on the wrong Kubernetes cluster or with the wrong AWS permissions.

jmgrady

Reviewable status: 10 of 12 files reviewed, all discussions resolved (waiting on @jmgrady)

deploy/scripts/utils.py line 70 at r4 (raw file):

Previously, jmgrady (Jim Grady) wrote…

No, it is not (and it doesn't match the description!

I have added a test for the case you mentioned at the beginning of the function:
    if len(options) == 1 and curr_selection is not None and curr_selection == options[0]:
       return curr_selection 
If this condition is not met, it will prompt the user if len(options) >= 1 instead of strictly > so if the curr_selection is None the user will be prompted to select the option. I don't want to blindly take the single option because if it is not the correct one, The Combine could be installed on the wrong Kubernetes cluster or with the wrong AWS permissions.

Done.

imnasnainaec

Reviewed 1 of 2 files at r5, all commit messages.
Reviewable status: 11 of 12 files reviewed, 2 unresolved discussions (waiting on @jmgrady)

deploy/scripts/utils.py line 80 at r5 (raw file):

                prompt_str = f"Enter {name}: "
            else:
                prompt_str = f"Enter {name}(Default: {curr_selection}): "

A space between {name} and (Default might be nice.

deploy/scripts/utils.py line 101 at r5 (raw file):

                            curr_selection = None
                    except ValueError:
                        curr_selection = None

It looks like an invalid user selection will cause the initial default to no longer be the default. If that's the case, is that okay?

jmgrady

Reviewable status: 11 of 12 files reviewed, 1 unresolved discussion (waiting on @imnasnainaec and @jmgrady)

deploy/scripts/utils.py line 80 at r5 (raw file):

Previously, imnasnainaec (D. Ror.) wrote…

A space between {name} and (Default might be nice.

Done.

deploy/scripts/utils.py line 101 at r5 (raw file):

Previously, imnasnainaec (D. Ror.) wrote…

It looks like an invalid user selection will cause the initial default to no longer be the default. If that's the case, is that okay?

I don't have a problem with that. By entering something the first time it is safe to assume that the user does not want the default so they just need to select a valid option. This also conforms to the usual practice that Ctrl-C will just exit completely in the case that the user does not want any of the suggestions.

I did, however, update it so that it will reprint the list if an invalid option is selected.

imnasnainaec

Reviewed 1 of 1 files at r6, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @jmgrady)

jmgrady added 11 commits September 6, 2022 11:22

Add method to confirm kubectl context

3ac92aa

Merge branch 'master' into kube-context-guard

394b086

Merge branch 'master' into kube-context-guard

b887058

WIP: Validate kubernetes context on install

ad56949

Merge branch 'master' into kube-context-guard

aef556d

Merge branch 'master' into kube-context-guard

c27da6a

Merge branch 'master' into kube-context-guard

9985b25

Python formatting

e99fd14

Set development server name to thecombine.localhost

e2c2f10

Add function to verify required environment variables are set

89e7dc0

Merge branch 'master' into kube-context-guard

15386ca

jmgrady added deployment python kubernetes CI/CD labels Sep 28, 2022

jmgrady self-assigned this Sep 28, 2022

jmgrady added 4 commits October 7, 2022 11:17

Update Python dependencies

fac3b45

Improve user feedback importing semantic domains

64c9a38

Confirm kubernetes context & AWS environment when installing The Combine

23e61e2

Fix Python formatting and type hints

247af5c

jmgrady requested a review from imnasnainaec October 11, 2022 14:24

jmgrady marked this pull request as ready for review October 11, 2022 14:24

imnasnainaec reviewed Oct 11, 2022

View reviewed changes

imnasnainaec and others added 2 commits October 11, 2022 13:44

Export entry id: vern -> vern_guid (#1740)

fd37413

Make LiftEntry ids unique.

Updates from review

0b1d319

jmgrady commented Oct 11, 2022

View reviewed changes

Merge branch 'master' into kube-context-guard

14720b6

imnasnainaec reviewed Oct 12, 2022

View reviewed changes

Update from review

b0b171c

jmgrady commented Oct 12, 2022

View reviewed changes

Python reformat

d4e369b

imnasnainaec reviewed Oct 12, 2022

View reviewed changes

Update from review

2f0fc26

jmgrady commented Oct 12, 2022

View reviewed changes

imnasnainaec approved these changes Oct 13, 2022

View reviewed changes

jmgrady merged commit 0498b7b into master Oct 13, 2022

jmgrady deleted the kube-context-guard branch October 13, 2022 09:02

jmgrady mentioned this pull request Oct 19, 2022

Improve reliability of setup_combine.py #1706

Closed

jmgrady mentioned this pull request Nov 1, 2022

Correct reCAPTCHA key for thecombine.localhost #1753

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Verify Kubernetes context before install #1739

Verify Kubernetes context before install #1739

jmgrady commented Sep 28, 2022 •

edited by jasonleenaylor

Loading

codecov-commenter commented Sep 28, 2022 •

edited

Loading

imnasnainaec left a comment

jmgrady left a comment

imnasnainaec left a comment

jmgrady left a comment

jmgrady left a comment

imnasnainaec left a comment

jmgrady left a comment

imnasnainaec left a comment

Verify Kubernetes context before install #1739

Verify Kubernetes context before install #1739

Conversation

jmgrady commented Sep 28, 2022 • edited by jasonleenaylor Loading

codecov-commenter commented Sep 28, 2022 • edited Loading

Codecov Report

imnasnainaec left a comment

Choose a reason for hiding this comment

jmgrady left a comment

Choose a reason for hiding this comment

imnasnainaec left a comment

Choose a reason for hiding this comment

jmgrady left a comment

Choose a reason for hiding this comment

jmgrady left a comment

Choose a reason for hiding this comment

imnasnainaec left a comment

Choose a reason for hiding this comment

jmgrady left a comment

Choose a reason for hiding this comment

imnasnainaec left a comment

Choose a reason for hiding this comment

jmgrady commented Sep 28, 2022 •

edited by jasonleenaylor

Loading

codecov-commenter commented Sep 28, 2022 •

edited

Loading