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

doc: miniojob doc #2173

Merged
merged 11 commits into from
Jun 28, 2024
Merged

doc: miniojob doc #2173

merged 11 commits into from
Jun 28, 2024

Conversation

jiuker
Copy link
Contributor

@jiuker jiuker commented Jun 21, 2024

[miniojob doc](doc: miniojob doc)

miniojob doc
docs/minio-job.md Outdated Show resolved Hide resolved
docs/minio-job.md Outdated Show resolved Hide resolved
docs/minio-job.md Outdated Show resolved Hide resolved
docs/minio-job.md Outdated Show resolved Hide resolved
docs/minio-job.md Outdated Show resolved Hide resolved
docs/minio-job.md Outdated Show resolved Hide resolved
docs/minio-job.md Outdated Show resolved Hide resolved
docs/minio-job.md Outdated Show resolved Hide resolved
docs/minio-job.md Outdated Show resolved Hide resolved
docs/minio-job.md Outdated Show resolved Hide resolved
Apply suggestions from code review

Co-authored-by: Shubhendu <[email protected]>
@jiuker jiuker requested a review from shtripat June 21, 2024 06:05
shtripat
shtripat previously approved these changes Jun 21, 2024
Copy link
Contributor

@shtripat shtripat left a comment

Choose a reason for hiding this comment

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

LGTM

docs/minio-job.md Show resolved Hide resolved
docs/minio-job.md Outdated Show resolved Hide resolved
docs/minio-job.md Show resolved Hide resolved
docs/minio-job.md Show resolved Hide resolved
docs/minio-job.md Show resolved Hide resolved
docs/minio-job.md Outdated Show resolved Hide resolved
docs/minio-job.md Outdated Show resolved Hide resolved
@jiuker jiuker requested a review from ramondeklein June 21, 2024 08:33
docs/minio-job.md Outdated Show resolved Hide resolved
docs/minio-job.md Show resolved Hide resolved
docs/minio-job.md Show resolved Hide resolved
docs/minio-job.md Show resolved Hide resolved
docs/minio-job.md Outdated Show resolved Hide resolved
docs/minio-job.md Outdated Show resolved Hide resolved
suggestion
docs/minio-job.md Outdated Show resolved Hide resolved
@jiuker jiuker requested review from cniackz and cesnietor June 21, 2024 14:36
@pjuarezd
Copy link
Member

@ravindk89 @feorlen would you mind checking on this please?

@ravindk89
Copy link
Contributor

Sure thing!

@jiuker jiuker requested review from ramondeklein and shtripat June 22, 2024 14:09
@jiuker jiuker self-assigned this Jun 22, 2024
jiuker added 2 commits June 23, 2024 01:15
@harshavardhana
Copy link
Member

PTAL @ravindk89 @djwfyi @feorlen

Copy link
Contributor

@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.

Mostly clarifying questions on how parts of this would/should work.

docs/minio-job.md Show resolved Hide resolved
docs/minio-job.md Outdated Show resolved Hide resolved
docs/minio-job.md Show resolved Hide resolved
docs/minio-job.md Show resolved Hide resolved
Comment on lines 183 to 186
### command
The `command` field specifies the command that will be executed by the `mc` command.
`args` must be empty. And `op` can be set to the main command name.
```
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this for arbitrary commands? Just an array list of strings to pass into a constructor, so I could do

- op: config
- command:
   - "mc"
   - "admin"
   - "config"
   - "set"
   - "config_setting"
   - "config_param=foo"
   - "config_param=bar"

Is this ballpark?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yesh

Copy link
Contributor

@ramondeklein ramondeklein Jun 25, 2024

Choose a reason for hiding this comment

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

I would still like to remove op in case command is used (it has no use in that case). Or use op: command and use the args as the command arguments. I think the current syntax is confusing.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agree with ramon - it's sufficient to support name since that is what you would use dependsOn with. op is fully redundant.

Copy link
Contributor

Choose a reason for hiding this comment

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

As a note - how is this meaningfully different from the pre-baked paths?

I could also do

- op: mb
- command:
  - "mc"
  - "mb"
  - "--with-locks"
  - "myminio"
  - "foobar"

Right?

Also - assuming myminio is a preconfigured alias for the Tenant. Otherwise we have to specify what alias to use here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. myminio is a preconfigured alias for the Tenant. Have a documentation at this markdown top line now.
Nothing different. @ravindk89

Copy link
Contributor Author

@jiuker jiuker Jun 26, 2024

Choose a reason for hiding this comment

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

op is optional now for this.
3fce527

docs/minio-job.md Show resolved Hide resolved
add flags example
docs/minio-job.md Show resolved Hide resolved
docs/minio-job.md Outdated Show resolved Hide resolved
docs/minio-job.md Show resolved Hide resolved
docs/minio-job.md Show resolved Hide resolved
Comment on lines 31 to 32
USER: ZGFuaWVs
PASSWORD: ZGFuaWVsMTIz
Copy link
Contributor

Choose a reason for hiding this comment

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

We may want to add a comment in YAML that this specifies user daniel with password: daniel123.

docs/minio-job.md Outdated Show resolved Hide resolved
docs/minio-job.md Show resolved Hide resolved
docs/minio-job.md Show resolved Hide resolved
Comment on lines 183 to 186
### command
The `command` field specifies the command that will be executed by the `mc` command.
`args` must be empty. And `op` can be set to the main command name.
```
Copy link
Contributor

@ramondeklein ramondeklein Jun 25, 2024

Choose a reason for hiding this comment

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

I would still like to remove op in case command is used (it has no use in that case). Or use op: command and use the args as the command arguments. I think the current syntax is confusing.

docs/minio-job.md Show resolved Hide resolved
jiuker and others added 5 commits June 26, 2024 10:05
@jiuker jiuker requested a review from ramondeklein June 26, 2024 02:36
docs/minio-job.md Show resolved Hide resolved
docs/minio-job.md Show resolved Hide resolved
docs/minio-job.md Show resolved Hide resolved
Copy link
Contributor

@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.

OK from docs perspective this is a good baseline, and as Pedro noted the goal here is baseline, not complete.

As we iterate we can update this doc, which will eventually flow into the webdoc content.

@jiuker jiuker requested a review from ramondeklein June 28, 2024 00:33
@harshavardhana harshavardhana merged commit 8a1eab5 into minio:master Jun 28, 2024
31 checks passed
@djwfyi djwfyi mentioned this pull request Jul 18, 2024
13 tasks
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.

6 participants