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

Ability to set azure cluster composition #323

Merged
merged 15 commits into from
Jun 19, 2020

Conversation

harshashah16
Copy link
Contributor

No description provided.

@harshashah16 harshashah16 marked this pull request as ready for review June 15, 2020 02:20
chattarajoy
chattarajoy previously approved these changes Jun 15, 2020
bin/qds.py Outdated Show resolved Hide resolved
@@ -5,3 +5,9 @@ def create_parser(self, argparser):

def set_cloud_config_from_arguments(self, arguments):
return NotImplemented

def set_composition_arguments(self, comp_group):
pass
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

Suggested change
pass
return NotImplemented

?

Copy link
Contributor Author

@harshashah16 harshashah16 Jun 15, 2020

Choose a reason for hiding this comment

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

If we go with that approach, we have to check for cloud when we set the composition.
Rather than having an if condition to check whether the cloud supports, each cloud can set empty data or implement the function which will sets the value.
I implemented it this way to avoid that if conditions. In future, user has to implement the function and no need to edit the condition to allow cloud to set compositions.

Co-authored-by: Joy Lal Chattaraj <8450903+chattarajoy@users.noreply.github.com>
chattarajoy
chattarajoy previously approved these changes Jun 17, 2020
@chattarajoy
Copy link
Member

LGTM overall, @harshshah87 please let me know if this is ready to merge

akaranjkar-qu
akaranjkar-qu previously approved these changes Jun 18, 2020
qds_sdk/qubole.py Outdated Show resolved Hide resolved
bin/qds.py Outdated Show resolved Hide resolved
@harshashah16
Copy link
Contributor Author

@chattarajoy : We can merge now.

@chattarajoy chattarajoy merged commit 96d16fd into qubole:unreleased Jun 19, 2020
chattarajoy pushed a commit that referenced this pull request Oct 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants