-
Notifications
You must be signed in to change notification settings - Fork 49
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
add machineset resource implementation #97
Conversation
ocp_resources/machineset.py
Outdated
|
||
def to_dict(self): | ||
|
||
data = super().to_dict() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please rename data
to res
, keep our code the same in all resources.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ack
ocp_resources/machineset.py
Outdated
"labels", | ||
"template", | ||
) | ||
_provider_spec = "providerSpec" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why this one is separated from the above?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
apparently the pre-commit changed that before my commit, ill change that.
ocp_resources/machineset.py
Outdated
@@ -0,0 +1,91 @@ | |||
from ocp_resources.machine import NamespacedResource |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please import from resource
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ack
ocp_resources/machineset.py
Outdated
f"{self.api_group}/cluster-api-machine-type": self.machine_type, | ||
} | ||
|
||
data.setdefault(_spec, {}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
data.setdefault(_spec, {})
data[_spec]["replicas"] = self.replicas
can be done in one line
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ack
ocp_resources/machineset.py
Outdated
|
||
data.setdefault(_spec, {}) | ||
data[_spec]["replicas"] = self.replicas | ||
data[_spec].setdefault(_selector, {}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ack
71be86a
to
9238a17
Compare
ocp_resources/machineset.py
Outdated
"providerSpec", | ||
) | ||
|
||
res[_metadata].setdefault( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
res[_metadata].setdefault(_labels, {}).update(
{
f"{self.api_group}/cluster-api-cluster": self.cluster_name,
f"{self.api_group}/cluster-api-machine-role": self.machine_role,
f"{self.api_group}/cluster-api-machine-type": self.machine_type,
},
})
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what's the difference between those two statements?
res[_metadata].setdefault(
_labels,
{
f"{self.api_group}/cluster-api-cluster": self.cluster_name,
f"{self.api_group}/cluster-api-machine-role": self.machine_role,
f"{self.api_group}/cluster-api-machine-type": self.machine_type,
},
)
This statement will only overrun the labels in case the "lables" key does not appear in the dict.
So if the user applied its own labels then i will not overrun it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you want to update the existing one if any.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ack
9238a17
to
8d0f527
Compare
ocp_resources/machineset.py
Outdated
} | ||
) | ||
res.setdefault( | ||
_spec, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto, use update()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ack
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.
8d0f527
to
bf2cb4f
Compare
ocp_resources/machineset.py
Outdated
|
||
res[_metadata].setdefault(_labels, {}).update( | ||
{ | ||
f"{self.api_group}/cluster-api-cluster": self.cluster_name, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cluster-api-cluster
please save and re-use (applies to all those which are used more than once)- How about saving all in one dict and use dict comprehension to add only the relevant data in each case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- ack
- can you share an example of what you mean?
bf2cb4f
to
7a49a64
Compare
ocp_resources/machineset.py
Outdated
|
||
res.setdefault(_spec, {}).update( | ||
{ | ||
"replicas": self.replicas, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you need setdefault with update for each one.
replicas
selector and so on
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ack, updated
7a49a64
to
6349f5e
Compare
6349f5e
to
4e8d29b
Compare
ocp_resources/machineset.py
Outdated
|
||
def __enter__(self): | ||
super().__enter__() | ||
if self.wait_ready_state and not self.wait(timeout=self.replicas * TIMEOUT): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not part of the creation, if you need to wait for some state, do it in a function.
enter is called when the user do with ....
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ack, yes i figured it might be nice to give the user an option to wait for the machine-set state while using the context manager, but it might be better to split that
ocp_resources/machineset.py
Outdated
bool: True if updating the machine-set was successful, False otherwise. | ||
""" | ||
LOGGER.info(f"Update machine-set {self.name} to {patches}") | ||
ResourceEditor(patches={self: patches}).update() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you don't need to call ResourceEditor from inside a resource
check how we do it in Deployment.scale_replicas
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ack
ocp_resources/machineset.py
Outdated
"cluster-api-machineset", | ||
) | ||
|
||
res[_metadata].setdefault(_labels, {}).update( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Discard my previous comments please, make is simple
no need to use setdefault(), just build you dict as needed.
we can assume that anyone that cal to_dict() create new resource.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ack
4e8d29b
to
2a3fb47
Compare
ocp_resources/machineset.py
Outdated
if ready_replicas and ready_replicas == self.desired_replicas: | ||
return True | ||
except TimeoutExpiredError: | ||
LOGGER.error( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please fit all errors in one LOGGER.error
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ack
ocp_resources/machineset.py
Outdated
def provider_spec_value(self): | ||
return self.instance.spec.template.spec.providerSpec.value | ||
|
||
def wait(self, timeout=TIMEOUT, sleep=1): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wait
please rename, this function wait for replicas
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ack
ocp_resources/machineset.py
Outdated
) | ||
return False | ||
|
||
def scale(self, replicas, wait_timeout=TIMEOUT, sleep=1, wait=True): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
scale
ditto rename to scale_replicas
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ack
2a3fb47
to
6ae5abf
Compare
ocp_resources/machineset.py
Outdated
@@ -0,0 +1,189 @@ | |||
import logging |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please rename to machine_set.py
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ack
ocp_resources/machineset.py
Outdated
wait (bool): True if waiting for machine-set to reach into 'ready' state, False otherwise. | ||
|
||
Returns: | ||
bool: True if scaling the machine set was successful, False otherwise. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Returns True also if wait=False (and not necessarily if scaling succeeded)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think its necessary to add it to the docstring.
basically if the update function didn't raise an exception, then that means the update was successful.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if wait=False, wait_for_replicas is not called.
A user should know from the docstring what is the expected behavior; for example:
bool: True if scaling the machine set was successful or wait=False, False otherwise.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ack
6ae5abf
to
b4a46ce
Compare
b4a46ce
to
f534c78
Compare
This patch adds the abilty to create the 'machineset' resource.
f534c78
to
34fc622
Compare
Kudos, SonarCloud Quality Gate passed!
|
/approve |
/verified |
/approve |
This patch adds the abilty to create the 'machineset' resource.
Short description:
More details:
What this PR does / why we need it:
Which issue(s) this PR fixes:
Special notes for reviewer:
Bug: