Skip to content

Ability to set azure cluster composition#323

Merged
chattarajoy merged 15 commits intoqubole:unreleasedfrom
harshashah16:azure-2
Jun 19, 2020
Merged

Ability to set azure cluster composition#323
chattarajoy merged 15 commits intoqubole:unreleasedfrom
harshashah16:azure-2

Conversation

@harshashah16
Copy link
Copy Markdown
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
return NotImplemented

def set_composition_arguments(self, comp_group):
pass
Copy link
Copy Markdown
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
Copy Markdown
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 <[email protected]>
chattarajoy
chattarajoy previously approved these changes Jun 17, 2020
@chattarajoy
Copy link
Copy Markdown
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
@harshashah16
Copy link
Copy Markdown
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