Skip to content
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

Issue #79 Read app configuration file once in app.py, and pass parame… #90

Merged
merged 1 commit into from
Jun 14, 2022

Conversation

kburke
Copy link

@kburke kburke commented Jun 10, 2022

…ters to worker for it to use what it needs. Issue #80 Change Globus groups file override indication to be indicated by a file name.

Sorry to put two cards in one PR, but the work was kind of muddle together.

…ters to worker for it to use what it needs. Issue #80 Change Globus groups file override indication to be indicated by a file name.
Copy link
Collaborator

@yuanzhou yuanzhou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kburke the changes look good to me. The only comment I have is should we remove https://github.com/hubmapconsortium/uuid-api/blob/kburke/uuidSchemaRevisions/src/instance/globus-groups.json from github to avoid confusion? HuBMAP uses the groups json file from commons and we won't be passing in a new one via uuid-api. For SenNet and BCRF projects, they'll use a separate one, and they'll probably need to be committed to github.

@kburke
Copy link
Author

kburke commented Jun 14, 2022

@yuanzhou In the original writeup of issue #80, I put removing the file from Git based on @shirey comments during a meeting. But he later revised the issue to say, "L_ocating the GROUPS info file in the src/instance/ directory alongside app.cfg seems like the appropriate place for it, but ignored by git as app.cfg is_.", as archived in this comment.

Based on that, I revised the description to say keep it in GitHub. But if his first instinct was to get it out of there, and that is yours also, maybe it should go.

@yuanzhou yuanzhou merged commit b265ffd into dev-integrate Jun 14, 2022
@shirey
Copy link
Member

shirey commented Jun 15, 2022

@yuanzhou and @kburke My preference would be to still track the SenNet (and eventual BCRF) globus_groups files in GitHub. There is nothing sensitive in these. Maybe just change the name of this one to sennet_globus_groups.json for now and eventually we'll move it to a SenNet repo once we figure out the cross organization handling of UUID (and other) services.

@kburke
Copy link
Author

kburke commented Jun 15, 2022

I can push a file name change and app.py change. @shirey is there someone else who should align the content with the SenNet group info, rather than HuBMAP? It is unclear to me if description, name, displayname, etc. content referencing HM is important, or only the identfiers.

@shirey
Copy link
Member

shirey commented Jun 16, 2022

@kburke Sorry, based on our recent conversation leave this file as is- my confusion abounds. @yuanzhou No further changes coming for this PR (yet :))

@kburke kburke added the P Pitt dev team label Jun 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P Pitt dev team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants