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.

Sign up for GitHub

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

Jump to bottom

feat(config) added global config manager #533

Merged
merged 1 commit into from
May 27, 2019
Merged

feat(config) added global config manager #533

merged 1 commit into from
May 27, 2019

Conversation

jsam
Copy link
Contributor

@jsam jsam commented May 7, 2019 •
edited

closes #532

@jsam jsam requested review from ableuler and rokroskar May 20, 2019 16:38
@jsam jsam marked this pull request as ready for review May 20, 2019 16:39
@jsam jsam requested a review from a team as a code owner May 20, 2019 16:39
ableuler
ableuler suggested changes May 21, 2019
View reviewed changes
Copy link
Contributor

@ableuler ableuler left a comment •
edited

Choose a reason for hiding this comment

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

One small change suggested, rest looks good to me 👍

renku/cli/_config.py Outdated Show resolved Hide resolved
@jsam jsam requested a review from ableuler May 21, 2019 08:30
@rokroskar
Copy link
Member

I don't see any CLI for this?

@rokroskar
Copy link
Member

Note that the current config CLI was implemented to allow the user to specify a custom image registry that is used when pulling an image locally. This isn't used very much atm, but we should decide if it's worth keeping. In general, we will be faced with the problem of some configs belonging in the repo (i.e. used by everyone who might be sharing a project) and other configs being strictly for a single user (i.e. secrets). How should we deal with this?

rokroskar
rokroskar reviewed May 21, 2019
View reviewed changes
renku/api/client.py Outdated Show resolved Hide resolved
rokroskar
rokroskar requested changes May 21, 2019
View reviewed changes
Copy link
Member

@rokroskar rokroskar left a comment •
edited by jsam

Choose a reason for hiding this comment

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

As discussed IRL:

  • implement renku config --global / keep the git config code
  • remove dangling code from /cli/_config.py
  • remove /cli/env.py and /cli/endpoint.py
  • implement ConfigManager as another mixin

ableuler
ableuler previously approved these changes May 21, 2019
View reviewed changes
@rokroskar
Copy link
Member

Is this ready for re-review @jsam?

@jsam jsam requested a review from rokroskar May 23, 2019 07:44
rokroskar
rokroskar reviewed May 23, 2019
View reviewed changes
renku/api/config.py Outdated Show resolved Hide resolved
rokroskar
rokroskar requested changes May 23, 2019
View reviewed changes
Copy link
Member

@rokroskar rokroskar left a comment

Choose a reason for hiding this comment

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

Nice, thanks! just minor suggestions

renku/api/config.py Outdated Show resolved Hide resolved
tests/test_cli.py Show resolved Hide resolved
renku/api/config.py Outdated

def store_config(self, config):
"""Persists global configuration object."""
with open(str(self.config_path), 'w') as file:
Copy link
Member

Choose a reason for hiding this comment

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

should we be setting some restrictive access flags on the file and directory, e.g. 600?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So currently they are 644. Why is 644 a problem?

Copy link
Member

Choose a reason for hiding this comment

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

multi-tenant environments, e.g. HPC clusters

@jsam jsam requested a review from rokroskar May 24, 2019 09:16
rokroskar
rokroskar previously approved these changes May 27, 2019
View reviewed changes
Copy link
Member

@rokroskar rokroskar left a comment

Choose a reason for hiding this comment

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

🎉

@jsam jsam merged commit 938f820 into SwissDataScienceCenter:master May 27, 2019
@jsam jsam deleted the 532-config branch May 27, 2019 11:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ableuler ableuler ableuler approved these changes

@rokroskar rokroskar rokroskar left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Configuration manager
3 participants
@jsam @rokroskar @ableuler