Add check-local-portal-creds#244
Conversation
| if not appname: | ||
| if 'cgap' in REPO_NAME: | ||
| appname = 'cgap' | ||
| elif 'ff' in REPO_NAME or 'ffourfront' in REPO_NAME: |
There was a problem hiding this comment.
Do you mean 'fourfront' not 'ffourfront'?
There was a problem hiding this comment.
Yep. Thanks for spotting that.
There was a problem hiding this comment.
@dmichaels-harvard, I re-implemented some details of this PR in a more data-driven style to make it harder for such typos to creep in. The whole thing was too dependent on incidental string constants. See commit de10c89 for details.
There was a problem hiding this comment.
OK and commit b1c291d adds a bit more change.
willronchetti
left a comment
willronchetti
left a comment
There was a problem hiding this comment.
Approving as I think the code itself is fine but I think it's still an open question whether this belongs here or in utils. I think utils makes more sense so the env bucket constants mentioned here can be bought in globally from there (instead of from snovault, which is not likely to be a useful import outside of the portals).
| CGAP_ENV_BUCKET = 'cgap-devtest-main-foursight-envs', | ||
| FOURFRONT_ENV_BUCKET = 'foursight-prod-envs' |
There was a problem hiding this comment.
The reason why I think this makes sense in utils rather than snovault is because these details already sort of exist there vs. here they are out of place IMO as I don't like creating a dependency on environments within snovault, feels circular.
| APP_NAMES = [APP_CGAP, APP_FOURFRONT] | ||
|
|
||
| APP_BUCKET_MAPPINGS = { | ||
| APP_CGAP: CGAP_ENV_BUCKET, | ||
| APP_FOURFRONT: FOURFRONT_ENV_BUCKET, | ||
| PSEUDO_APP_SNOVAULT: FOURFRONT_ENV_BUCKET, # doesn't have a bucket of its own | ||
| } |
There was a problem hiding this comment.
I would prefer a structure that infers this from the calling repository, maybe by reading a file?
|
|
||
| def main(): | ||
| parser = argparse.ArgumentParser( | ||
| description='Echos version information from ~/.cgap-keys.json or override file.') |
There was a problem hiding this comment.
This description seems lifted from somewhere else
|
I pushed up a fresh copy of this but then realized we installed a similar script in |
3 participants
check-local-portal-credscommand.Makefile:test-fulltarget (liketestbut without instafail)