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

kubectl minio init command arguments update #835

Merged
merged 2 commits into from
May 3, 2023

Conversation

pjuarezd
Copy link
Member

Documentation update

closes minio/operator#1568

Copy link
Collaborator

@ravindk89 ravindk89 left a comment

Choose a reason for hiding this comment

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

A few change requests. I think we might be able to cheat a bit if we know for certain that the constants.go file is always what we reference when setting defaults.

@@ -83,7 +83,7 @@ The command supports the following flags:
:optional:

The image to use when deploying the :minio-git:`MinIO Console <console>` in Operator mode, where administrators can create and manage MinIO tenants using a Graphical User Interface.
Defaults to ``minio/console:v0.17.3``.
Defaults to the :minio-git:`latest release of the operator <operator/releases/latest>`.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
Defaults to the :minio-git:`latest release of the operator <operator/releases/latest>`.
Defaults to the version bundled with the `operator release version <operator/releases/latest>`__ matching the plugin version.

Since someone could use this doc with an older kubectl minio version, and the console version would be whatever is attached to that version.

I don't think it's necessary to keep a matrix, just mention they are bundled.

Copy link
Member Author

Choose a reason for hiding this comment

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

mentioned the variable that holds the value

.. mc-cmd:: --default-kes-image
:optional:

The default :minio-git:`kes <kes>` image to use when creating a new MinIO tenant.
Defaults to ``minio/kes:v0.18.0``.
Defaults to the :minio-git:`latest release of KES <kes/releases/latest>`.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same comment - e.g. https://github.com/minio/operator/blob/v5.0.4/kubectl-minio/cmd/helpers/constants.go

So it's not guaranteed to be current latest, just whatever is associated to that release.

If this file is generally the right place to look, we could cheat a little bit in the conf and auto-inject the latest release tag to make it easier to link.

Otherwise, it's enough to modify similar to suggestion before, e.g. "defaults to the version bundled in the matching Operator release. See for more information"

Copy link
Member Author

Choose a reason for hiding this comment

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

right, default value will always be whatever is on the constants.go file, on this case the DefaultKESImage env variable.

do you have an example on how could I auto-inject the value from a file?

Copy link
Collaborator

@ravindk89 ravindk89 Apr 26, 2023

Choose a reason for hiding this comment

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

I have to play with this a bit - I'm not sure if there's an easy way to pull and parse that file.

Right now in Makefile we have a dependency builder that snags stuff with wget and parses it from there.

it would be nicer if this was a YAML of all of our defaults/constants, but that is for later.

For right now we can maybe say...

Defaults to the `version bundled in the matching Operator release <https://github.com/minio/operator/blob/master/kubectl-minio/cmd/helpers/constants.go>`__

Copy link
Member Author

Choose a reason for hiding this comment

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

applied, and mentioned the variable that holds the value

@pjuarezd pjuarezd force-pushed the kubectl-minio-init-arguments-update branch from ead2888 to c5f35d0 Compare May 2, 2023 19:25
Copy link
Collaborator

@djwfyi djwfyi left a comment

Choose a reason for hiding this comment

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

Three fixes for Sphinx links, otherwise LGTM.
I'll do those real quick.

Fixes links by adding the required trailing underscores.
Copy link
Collaborator

@djwfyi djwfyi left a comment

Choose a reason for hiding this comment

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

LGTM

@djwfyi djwfyi merged commit 21e14b0 into main May 3, 2023
@djwfyi djwfyi deleted the kubectl-minio-init-arguments-update branch May 3, 2023 15:35
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.

Minio kubelet plugin can't start console docker container
3 participants