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

enhance(container): enable setting user #199

Merged
merged 5 commits into from
Sep 27, 2021
Merged

Conversation

JordanSussman
Copy link
Collaborator

@JordanSussman JordanSussman requested a review from a team as a code owner September 24, 2021 20:06
@codecov
Copy link

codecov bot commented Sep 24, 2021

Codecov Report

Merging #199 (6f1ee8c) into master (c8edeac) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #199   +/-   ##
=======================================
  Coverage   96.97%   96.98%           
=======================================
  Files          53       53           
  Lines        5462     5465    +3     
=======================================
+ Hits         5297     5300    +3     
  Misses        119      119           
  Partials       46       46           
Impacted Files Coverage Δ
pipeline/container.go 94.39% <100.00%> (+0.02%) ⬆️
yaml/service.go 90.16% <100.00%> (+0.16%) ⬆️
yaml/step.go 95.38% <100.00%> (+0.07%) ⬆️

Copy link
Contributor

@jbrockopp jbrockopp left a comment

Choose a reason for hiding this comment

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

The code you have written LGTM! 🎉

However, I noticed that the story linked refers to enabling this for both steps and services.

If I'm following correctly, I believe this adds YAML support for the user key for a step, but not for a service.

Should there also be code here for adding a User string field to the Service struct?

types/yaml/service.go

Lines 21 to 32 in c8edeac

// Service is the yaml representation
// of a Service in a pipeline.
// nolint:lll // jsonschema will cause long lines
Service struct {
Image string `yaml:"image,omitempty" json:"image,omitempty" jsonschema:"required,minLength=1,description=Docker image used to create ephemeral container.\nReference: https://go-vela.github.io/docs/reference/yaml/services/#the-image-tag"`
Name string `yaml:"name,omitempty" json:"name,omitempty" jsonschema:"required,minLength=1,description=Unique identifier for the container in the pipeline.\nReference: https://go-vela.github.io/docs/reference/yaml/services/#the-name-tag"`
Entrypoint raw.StringSlice `yaml:"entrypoint,omitempty" json:"entrypoint,omitempty" jsonschema:"description=Commands to execute inside the container.\nReference: https://go-vela.github.io/docs/reference/yaml/services/#the-entrypoint-tag"`
Environment raw.StringSliceMap `yaml:"environment,omitempty" json:"environment,omitempty" jsonschema:"description=Variables to inject into the container environment.\nReference: https://go-vela.github.io/docs/reference/yaml/services/#the-environment-tag"`
Ports raw.StringSlice `yaml:"ports,omitempty" json:"ports,omitempty" jsonschema:"description=List of ports to map for the container in the pipeline.\nReference: https://go-vela.github.io/docs/reference/yaml/services/#the-ports-tag"`
Pull string `yaml:"pull,omitempty" json:"pull,omitempty" jsonschema:"enum=always,enum=not_present,enum=on_start,enum=never,default=not_present,description=Declaration to configure if and when the Docker image is pulled.\nReference: https://go-vela.github.io/docs/reference/yaml/services/#the-pul-tag"`
Ulimits UlimitSlice `yaml:"ulimits,omitempty" json:"ulimits,omitempty" jsonschema:"description=Set the user limits for the container.\nReference: https://go-vela.github.io/docs/reference/yaml/services/#the-ulimits-tag"`
}

@JordanSussman
Copy link
Collaborator Author

The code you have written LGTM! 🎉

However, I noticed that the story linked refers to enabling this for both steps and services.

If I'm following correctly, I believe this adds YAML support for the user key for a step, but not for a service.

Should there also be code here for adding a User string field to the Service struct?

types/yaml/service.go

Lines 21 to 32 in c8edeac

// Service is the yaml representation
// of a Service in a pipeline.
// nolint:lll // jsonschema will cause long lines
Service struct {
Image string `yaml:"image,omitempty" json:"image,omitempty" jsonschema:"required,minLength=1,description=Docker image used to create ephemeral container.\nReference: https://go-vela.github.io/docs/reference/yaml/services/#the-image-tag"`
Name string `yaml:"name,omitempty" json:"name,omitempty" jsonschema:"required,minLength=1,description=Unique identifier for the container in the pipeline.\nReference: https://go-vela.github.io/docs/reference/yaml/services/#the-name-tag"`
Entrypoint raw.StringSlice `yaml:"entrypoint,omitempty" json:"entrypoint,omitempty" jsonschema:"description=Commands to execute inside the container.\nReference: https://go-vela.github.io/docs/reference/yaml/services/#the-entrypoint-tag"`
Environment raw.StringSliceMap `yaml:"environment,omitempty" json:"environment,omitempty" jsonschema:"description=Variables to inject into the container environment.\nReference: https://go-vela.github.io/docs/reference/yaml/services/#the-environment-tag"`
Ports raw.StringSlice `yaml:"ports,omitempty" json:"ports,omitempty" jsonschema:"description=List of ports to map for the container in the pipeline.\nReference: https://go-vela.github.io/docs/reference/yaml/services/#the-ports-tag"`
Pull string `yaml:"pull,omitempty" json:"pull,omitempty" jsonschema:"enum=always,enum=not_present,enum=on_start,enum=never,default=not_present,description=Declaration to configure if and when the Docker image is pulled.\nReference: https://go-vela.github.io/docs/reference/yaml/services/#the-pul-tag"`
Ulimits UlimitSlice `yaml:"ulimits,omitempty" json:"ulimits,omitempty" jsonschema:"description=Set the user limits for the container.\nReference: https://go-vela.github.io/docs/reference/yaml/services/#the-ulimits-tag"`
}

Good catch! The user parameter was inadvertently left out of the original commit.

Copy link
Contributor

@jbrockopp jbrockopp left a comment

Choose a reason for hiding this comment

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

LGTM

@wass3r wass3r merged commit 522194f into go-vela:master Sep 27, 2021
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