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.

Sign up for GitHub

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

Jump to bottom

Clarify the use of buildah config options #592

Closed
wants to merge 1 commit into from
Closed

Clarify the use of buildah config options #592

wants to merge 1 commit into from

Conversation

rhatdan
Copy link
Member

@rhatdan rhatdan commented Apr 16, 2018

Make it clearer to the user which options can be executed
multiple times, versus those that can only used once.

Cleanup some documentation issues

Signed-off-by: Daniel J Walsh dwalsh@redhat.com

@rhatdan
Copy link
Member Author

rhatdan commented Apr 16, 2018

Fixed #591

TomSweeneyRedHat
TomSweeneyRedHat reviewed Apr 16, 2018
View reviewed changes
cmd/buildah/config.go Outdated
@@ -73,7 +73,7 @@ var (
},
cli.StringSliceFlag{
Name: "volume, v",
Usage: "add default `volume` path to be created for containers based on image",
Usage: "add default `volume` path to be created for containers based on image (default [])",
Copy link
Member

Choose a reason for hiding this comment

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

nit, looks like you've a double space before (default []).

TomSweeneyRedHat
TomSweeneyRedHat reviewed Apr 16, 2018
View reviewed changes
docs/buildah-config.md
@@ -13,24 +13,24 @@ Updates one or more of the settings kept for a container.

**--annotation** *annotation*

Adds an image *annotation* (e.g. annotation=*annotation*) to the image manifest
of any images which will be built using the specified container.
Add an image *annotation* (e.g. annotation=*annotation*) to the image manifest
Copy link
Member

Choose a reason for hiding this comment

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

could you whip up an example or three that shows how some of the multiples could be used? Perhaps an example for annotations, port and volume?

@rhatdan
Clarify the use of buildah config options
da4e9d4 
Make it clearer to the user which options can be executed
multiple times, versus those that can only used once.

Cleanup some documentation issues

Signed-off-by: Daniel J Walsh <dwalsh@redhat.com>
@rhatdan
Copy link
Member Author

rhatdan commented Apr 17, 2018

bot, retest this please

taqtiqa-mark
taqtiqa-mark reviewed Apr 17, 2018
View reviewed changes
docs/buildah-config.md
Specifies a *port* to expose when running containers based on any images which
will be built using the specified container.
Add a *port* to expose when running containers based on any images which
will be built using the specified container. Can be used multiple times.
Copy link

@taqtiqa-mark taqtiqa-mark Apr 17, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Can you clarify the syntax used in this Redhat documentation

# buildah config --port 80/tcp working-container

Are /tcp /udp and /icmp supported. If you use this decoration in the config do you have to use that when you launch, for example, a rkt container from the command line? For the googlers, rkt syntax (pre-spec) is --port 8080-tcp:8080

Copy link
Member Author

Choose a reason for hiding this comment

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

I would doubt you have to do anything special with rkt on the command line, although I have know clue.

@baude @TomSweeneyRedHat Could you verify the syntax.

Copy link

Choose a reason for hiding this comment

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

According to the spec the description should be:

Can be in the format of: port/tcp, port/udp, port with the default protocol being tcp if not specified. ICMP is not part of the OCI specification.

Hmm, maybe the documentation burden can be eased by linking to the version of the spec that Buildah is targeting. Example:

See OCI spec v1.0.1 Properties:ExposedPorts

If you're happy with that approach I'll add such a reference as I 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.

Sure lets merge this and then open a PR with these additional changes to docs.

@rhatdan
Copy link
Member Author

rhatdan commented Apr 17, 2018

@rh-atomic-bot r+

@rh-atomic-bot
Copy link
Collaborator

📌 Commit da4e9d4 has been approved by rhatdan

@rh-atomic-bot
Copy link
Collaborator

⚡ Test exempted: pull fully rebased and already tested.

@rhatdan
Copy link
Member Author

rhatdan commented Apr 17, 2018

@taqtiqa-mark Could you add a PR with the changes you want.

rhatdan added a commit to rhatdan/buildah that referenced this pull request Apr 18, 2018
@rhatdan
Clarify the use of buildah config options
3c01f91 
Make it clearer to the user which options can be executed
multiple times, versus those that can only used once.

Cleanup some documentation issues

Signed-off-by: Daniel J Walsh <dwalsh@redhat.com>

Closes: containers#592
Approved by: rhatdan

Signed-off-by: Daniel J Walsh <dwalsh@redhat.com>
rhatdan added a commit to rhatdan/buildah that referenced this pull request Apr 18, 2018
@rhatdan
Clarify the use of buildah config options
96662ea 
Make it clearer to the user which options can be executed
multiple times, versus those that can only used once.

Cleanup some documentation issues

Signed-off-by: Daniel J Walsh <dwalsh@redhat.com>

Closes: containers#592
Approved by: rhatdan

Signed-off-by: Daniel J Walsh <dwalsh@redhat.com>
rhatdan added a commit to rhatdan/buildah that referenced this pull request Apr 18, 2018
@rhatdan
Clarify the use of buildah config options
439c320 
Make it clearer to the user which options can be executed
multiple times, versus those that can only used once.

Cleanup some documentation issues

Signed-off-by: Daniel J Walsh <dwalsh@redhat.com>

Closes: containers#592
Approved by: rhatdan

Signed-off-by: Daniel J Walsh <dwalsh@redhat.com>
rhatdan added a commit to rhatdan/buildah that referenced this pull request Apr 18, 2018
@rhatdan
Clarify the use of buildah config options
d146011 
Make it clearer to the user which options can be executed
multiple times, versus those that can only used once.

Cleanup some documentation issues

Signed-off-by: Daniel J Walsh <dwalsh@redhat.com>

Closes: containers#592
Approved by: rhatdan

Signed-off-by: Daniel J Walsh <dwalsh@redhat.com>
rhatdan added a commit to rhatdan/buildah that referenced this pull request Apr 19, 2018
@rhatdan
Clarify the use of buildah config options
4958cc3 
Make it clearer to the user which options can be executed
multiple times, versus those that can only used once.

Cleanup some documentation issues

Signed-off-by: Daniel J Walsh <dwalsh@redhat.com>

Closes: containers#592
Approved by: rhatdan

Signed-off-by: Daniel J Walsh <dwalsh@redhat.com>
rhatdan added a commit to rhatdan/buildah that referenced this pull request Apr 19, 2018
@rhatdan
Clarify the use of buildah config options
deadd19 
Make it clearer to the user which options can be executed
multiple times, versus those that can only used once.

Cleanup some documentation issues

Signed-off-by: Daniel J Walsh <dwalsh@redhat.com>

Closes: containers#592
Approved by: rhatdan

Signed-off-by: Daniel J Walsh <dwalsh@redhat.com>
rh-atomic-bot pushed a commit that referenced this pull request Apr 19, 2018
@rhatdan @rh-atomic-bot
Clarify the use of buildah config options
e008b73 
Make it clearer to the user which options can be executed
multiple times, versus those that can only used once.

Cleanup some documentation issues

Signed-off-by: Daniel J Walsh <dwalsh@redhat.com>

Closes: #592
Approved by: rhatdan

Signed-off-by: Daniel J Walsh <dwalsh@redhat.com>

Closes: #602
Approved by: pixdrift
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 26, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@taqtiqa-mark taqtiqa-mark taqtiqa-mark left review comments

@TomSweeneyRedHat TomSweeneyRedHat TomSweeneyRedHat left review comments

Assignees
No one assigned
Labels
locked - please file new issue/PR
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@rhatdan @rh-atomic-bot @taqtiqa-mark @TomSweeneyRedHat