Use config.get() for safer access to configuration

[DukeDavis12]

This commit updates direct dictionary access of configuration with safer config.get().This change also prevents potential KeyError exceptions if the keys are missing.

Fixes #243, #252, #253 .

Description of the changes

How to test this PR?

---

This change is 

[mkow]

Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: )

---

-- commits line 5 at r1:
nitpick

Suggestion:

 config.get(). This

---

-- commits line 5 at r1:
Also? Isn't that the only thing it prevents?

Suggestion:

prevents

---

gsc.py line 352 at r1 (raw file):

    config = yaml.safe_load(args.config_file)
    if 'Image' in config.get('Gramine'):

This will still fail if Gramine key is not present in the config (you'll get a TypeError).

---

gsc.py line 353 at r1 (raw file):

    config = yaml.safe_load(args.config_file)
    if 'Image' in config.get('Gramine'):
        gramine_image_name = config.get('Gramine', {}).get('Image')

This still won't work if the name is missing, get_docker_image will get None instead of the name and fail.
We either have to provide a default value (which doesn't make sense for this config option, I think), or just error out on the key access (i.e. here) instead of somewhere deeper in the logic. It will be much clearer to the user if they get "Image" key not found error, instead of something saying that the Docker daemon got an unexpected None.

---

gsc.py line 474 at r1 (raw file):

    config = yaml.safe_load(args.config_file)
    if 'Image' in config.get('Gramine'):

ditto (TypeError)

[mkow]

@DukeDavis12: Please use Reviewable for the review and reply to my comments.

Reviewable status: 0 of 1 files reviewed, 5 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ) (waiting on @DukeDavis12)

[DukeDavis12]

Reviewable status: 0 of 1 files reviewed, 5 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ) (waiting on @mkow)

---

-- commits line 5 at r1:

Previously, mkow (Michał Kowalczyk) wrote…

nitpick

Done

---

-- commits line 5 at r1:

Previously, mkow (Michał Kowalczyk) wrote…

Also? Isn't that the only thing it prevents?

Done.

---

gsc.py line 352 at r1 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

This will still fail if Gramine key is not present in the config (you'll get a TypeError).

Now we are checking whether 'Gramine' key is available. If it is not available we are printing Missing Gramine key in config file.

[DukeDavis12]

Reviewable status: 0 of 1 files reviewed, 5 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ) (waiting on @mkow)

---

gsc.py line 353 at r1 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

This still won't work if the name is missing, get_docker_image will get None instead of the name and fail.
We either have to provide a default value (which doesn't make sense for this config option, I think), or just error out on the key access (i.e. here) instead of somewhere deeper in the logic. It will be much clearer to the user if they get "Image" key not found error, instead of something saying that the Docker daemon got an unexpected None.

Now we check for following cases:
Check if Gramine is present
Check if [Gramine][Image] is present or not
if not we check whether [Gramine][Repository] or [Gramine][Branch]
else we print the error.

---

gsc.py line 474 at r1 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

ditto (TypeError)

Done.

[DukeDavis12]

Done

Reviewable status: 0 of 1 files reviewed, 5 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ) (waiting on @mkow)

[donporter]

Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 8 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ) (waiting on @mkow)

---

gsc.py line 353 at r1 (raw file):

Previously, DukeDavis12 (Davis Benny) wrote…

Now we check for following cases:
Gramine section is present.
if Gramine is present
Check if [Gramine][Image] is present or not
if not we check whether [Gramine][Repository] or [Gramine][Branch]
else we print the error.

Nit: Can you save the result of the first config.get?

---

gsc.py line 474 at r1 (raw file):

Previously, DukeDavis12 (Davis Benny) wrote…

Done.

Nit: Reuse result of first config.get()

---

gsc.py line 354 at r2 (raw file):

    if config.get('Gramine') :
        if config.get('Gramine').get('Image'):
            gramine_image_name = config.get('Gramine', {}).get('Image')

Ditto

---

gsc.py line 358 at r2 (raw file):

                print(f'Cannot find base-Gramine Docker image `{gramine_image_name}`.')
                sys.exit(1)
        elif ((config.get('Gramine').get('Repository', None) is None)

And ditto

---

gsc.py line 359 at r2 (raw file):

                sys.exit(1)
        elif ((config.get('Gramine').get('Repository', None) is None)
              or (config.get('Gramine').get('Branch', None) is None)):

ditto

[DukeDavis12]

Reviewable status: 0 of 1 files reviewed, 8 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ) (waiting on @donporter and @mkow)

---

gsc.py line 353 at r1 (raw file):

Previously, donporter (Don Porter) wrote…

Nit: Can you save the result of the first config.get?

Done

---

gsc.py line 474 at r1 (raw file):

Previously, donporter (Don Porter) wrote…

Nit: Reuse result of first config.get()

Done.

---

gsc.py line 354 at r2 (raw file):

Previously, donporter (Don Porter) wrote…

Ditto

Done.

---

gsc.py line 358 at r2 (raw file):

Previously, donporter (Don Porter) wrote…

And ditto

Done.

---

gsc.py line 359 at r2 (raw file):

Previously, donporter (Don Porter) wrote…

ditto

Done.

[mkow]

Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 8 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ) (waiting on @donporter)

---

gsc.py line 359 at r3 (raw file):

                print(f'Cannot find base-Gramine Docker image `{gramine_image_name}`.')
                sys.exit(1)
        elif ((gramine_config.get('Repository') is None)

ditto after or

Suggestion:

elif ('Repository' not in gramine_config

---

gsc.py line 360 at r3 (raw file):

                sys.exit(1)
        elif ((gramine_config.get('Repository') is None)
              or (gramine_config.get('Branch') is None)):

Suggestion:

        elif ('Repository' not in gramine_config
             or 'Branch' not in gramine_config):

---

gsc.py line 361 at r3 (raw file):

        elif ((gramine_config.get('Repository') is None)
              or (gramine_config.get('Branch') is None)):
            print('Gramine.Image or Gramine.Repository or Gramine.Branch key not found')

Please split this into separate errors for better user experience (consider that before this PR the user would know which key exactly was missing, now it's coalesced).

---

gsc.py line 365 at r3 (raw file):

    else:
        print('Error: Missing `Gramine` key in config file.')
        sys.exit(1)

Please restructure this whole check. Instead of nesting it's clearer to check the prerequisites first, and then do the whole logic (i.e. "fail fast/early", instead of moving the fail cases to the else branches and nesting successes).

---

gsc.py line 485 at r3 (raw file):

    gramine_config = config.get('Gramine')
    if gramine_config:
        if gramine_config.get('Image'):

Suggestion:

    if 'Image' in config.get('Gramine', {}):

[kailun-qin]

@kailun-qin made 1 comment.
Reviewable status: all files reviewed, 8 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ) (waiting on donporter and DukeDavis12).

---

a discussion (no related file):
@DukeDavis12: Hi Davis, the issue has blocked several internal and external users, and I'd like to get it closed. Are you going to continue working on this PR and address the remaining comments? Otherwise, I can also help take over and finish it. Thanks!

[donporter]

@donporter reviewed 1 file and all commit messages, and resolved 3 discussions.
Reviewable status: all files reviewed, 5 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: OSCAR Lab) (waiting on DukeDavis12).

[kailun-qin]

@kailun-qin reviewed 1 file and all commit messages, and made 6 comments.
Reviewable status: all files reviewed, 5 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ) (waiting on mkow).

---

a discussion (no related file):

Previously, kailun-qin (Kailun Qin) wrote…

@DukeDavis12: Hi Davis, the issue has blocked several internal and external users, and I'd like to get it closed. Are you going to continue working on this PR and address the remaining comments? Otherwise, I can also help take over and finish it. Thanks!

I'm taking over this PR from Davis, as he's moved on to another project (see #238 (comment)).

---

gsc.py line 359 at r3 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

ditto after or

Done.

---

gsc.py line 361 at r3 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

Please split this into separate errors for better user experience (consider that before this PR the user would know which key exactly was missing, now it's coalesced).

Done.

---

gsc.py line 365 at r3 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

Please restructure this whole check. Instead of nesting it's clearer to check the prerequisites first, and then do the whole logic (i.e. "fail fast/early", instead of moving the fail cases to the else branches and nesting successes).

Done.

---

gsc.py line 360 at r3 (raw file):

                sys.exit(1)
        elif ((gramine_config.get('Repository') is None)
              or (gramine_config.get('Branch') is None)):

Done.

---

gsc.py line 485 at r3 (raw file):

    gramine_config = config.get('Gramine')
    if gramine_config:
        if gramine_config.get('Image'):

Done.

[donporter]

@donporter reviewed 1 file and all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: OSCAR Lab) (waiting on mkow).

[donporter]

Reviewable status: all files reviewed, 5 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: OSCAR Lab) (waiting on mkow).

[mkow]

@mkow reviewed 1 file and all commit messages, and resolved 5 discussions.
Reviewable status: complete! all files reviewed, all discussions resolved.