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

Adding Secret model and test #110

Merged
merged 8 commits into from
May 19, 2022
Merged

Adding Secret model and test #110

merged 8 commits into from
May 19, 2022

Conversation

portega-adv
Copy link
Contributor

Preparing the library to support changes required by fiaas/fiaas-deploy-daemon#178

@portega-adv portega-adv requested a review from a team as a code owner May 13, 2022 15:14
herodes1991
herodes1991 previously approved these changes May 13, 2022
url_template = "/api/v1/namespaces/{namespace}/secrets/{name}"

metadata = Field(ObjectMeta)
data = Field(dict)
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if it would be useful to also support the stringData field?
Preferably with a new field type to indicate that this is a write-only field. On the other hand, I don't think we have that anywhere else, so maybe it's not needed. Also, I'm not sure when the stringData field was introduced. Might cause problems if expecting to use it in a cluster that doesn't support it?

https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.19/#secret-v1-core

Copy link
Contributor Author

@portega-adv portega-adv May 16, 2022

Choose a reason for hiding this comment

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

Good question! The only use for this field is to provide not base64 encoded data, which will end up being merged into data field, so it's kind of redundant. Unless someone has a specific use case for this, I'd say it's not needed.

Copy link
Contributor

@herodes1991 herodes1991 May 16, 2022

Choose a reason for hiding this comment

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

Seems that we do not have any cluster with a k8s version without the stringData field so they will be no problem for add it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let me see if I can minimize the Codacy complains and I'll add it. Thanks for the feedback!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mortenlj Added field stringData. Please give it a look, I've created a new WriteOnlyField type, I think is what you expected.

Also, is there a way to get rid of those picky errors about the methods defined not as functions? I think is the proper way to do it and follows the same structure as the rest of the models.

Copy link
Member

Choose a reason for hiding this comment

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

Added field stringData. Please give it a look, I've created a new WriteOnlyField type, I think is what you expected.

👍

Also, is there a way to get rid of those picky errors about the methods defined not as functions? I think is the proper way to do it and follows the same structure as the rest of the models.

Technically, they could be moved to the top-level, outside the containing class, since it's not actually using anything from the class. Personally, I prefer to group tests in a class, but it's probably not necessary when there's only one class in the test file.

I guess you could decorate them with @staticmethod and see if that 1) works, 2) silences the linter. Or just move them to the top-level. Or just ignore them. 🤷

@portega-adv portega-adv merged commit 77e67e2 into master May 19, 2022
@portega-adv portega-adv deleted the add-secret branch May 19, 2022 07:48
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.

3 participants