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

add secrets property to build section #238

Merged
merged 4 commits into from Apr 11, 2022
Merged

add secrets property to build section #238

merged 4 commits into from Apr 11, 2022

Conversation

glours
Copy link
Contributor

@glours glours commented Mar 10, 2022

What this PR does / why we need it:
Add a secrets property to the build section which will allow Specification implementations to use secrets during the building step.

Which issue(s) this PR fixes:
Refer to #233

@glours glours self-assigned this Mar 10, 2022
This was referenced Mar 10, 2022
Open
Merged
ndeloof
ndeloof requested changes Mar 10, 2022
View reviewed changes
Copy link
Collaborator

@ndeloof ndeloof left a comment

Choose a reason for hiding this comment

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

LGTM with some minor adjustments

build.md Outdated Show resolved Hide resolved
schema/compose-spec.json Outdated
Comment on lines 106 to 118
"oneOf": [
{"type": "string"},
{
"type": "object",
"properties": {
"source": {"type": "string"},
"target": {"type": "string"},
"uid": {"type": "string"},
"gid": {"type": "string"},
"mode": {"type": "number"}
},
"additionalProperties": false,
"patternProperties": {"^x-": {}}
Copy link
Collaborator

Choose a reason for hiding this comment

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

this model is already declared twice for service configs and secrets, maybe this is a good opportunity to extract it into it's own definition and use a $ref

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 sure 👍

EricHripko
EricHripko reviewed Mar 10, 2022
View reviewed changes
Copy link
Collaborator

@EricHripko EricHripko left a comment

Choose a reason for hiding this comment

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

Thank you for looking into this! A few high-level questions first ❓

  • Does this allow for secrets to be optional?
  • What would the workflow look like for creating a secret? I'm aware that this crosses somewhat into implementation, but knowing the workflow E2E will help validate the schema changes.

@ndeloof
Copy link
Collaborator

ndeloof commented Mar 10, 2022

@EricHripko this is basically a port of docker build --secret id=mysecret,src=/local/secret that is consistent with the way docker compose exposes secrets as a bind mount. Obviously, this also can apply to platforms with native secret support, where user might have registered some secret and want them to be available during the build as /run/secret/id.

@EricHripko
Copy link
Collaborator

Thank you for confirming 👍 Is the idea that the user calls compose build --secret ... then?

@ndeloof
Copy link
Collaborator

ndeloof commented Mar 10, 2022

The idea is that use can declare an entry in secrets section in yaml file, and use a reference in service.build.secrets so that this secret is exposed during the build process (comparable to the same mechanism used when running services)
secret definition can either include a file (typically, some ~/.secrets-token) that each user will have to set, or could use an external: true secret for platforms with native support for secrets.

@glours glours requested a review from ndeloof March 10, 2022 11:32
@EricHripko
Copy link
Collaborator

That makes sense, thank you for clarifying 👍 One last question, would this still allow for optional secrets?

@glours glours mentioned this pull request Mar 15, 2022
Closed
glours and others added 3 commits March 31, 2022 15:54
@glours
add secrets property to build section
5eb35fc 
Signed-off-by: Guillaume Lours <guillaume.lours@docker.com>
@glours @ndeloof
Update when compose implementations should report an error with secre…
5253a4b 
…ts build property

Co-authored-by: Nicolas De loof <nicolas.deloof@gmail.com>
Signed-off-by: Guillaume Lours <guillaume.lours@docker.com>
@glours
define a json ref for service secrets and confids properties
c607167 
Signed-off-by: Guillaume Lours <guillaume.lours@docker.com>
ulyssessouza
ulyssessouza approved these changes Apr 1, 2022
View reviewed changes
Copy link
Contributor

@ulyssessouza ulyssessouza left a comment

Choose a reason for hiding this comment

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

LGTM

@ndeloof ndeloof mentioned this pull request Apr 11, 2022
Closed
@ndeloof
Copy link
Collaborator

ndeloof commented Apr 11, 2022

One last question, would this still allow for optional secrets?

The secrets mechanism define in the compose specification has no optional capability. This is something we could consider for a future improvements, but then would apply to all secrets, not just the build secrets

@ndeloof ndeloof merged commit 846cabc into compose-spec:master Apr 11, 2022
@EricHripko
Copy link
Collaborator

It would be great to allow for optional secrets, as development/CI/production environments can have different secrets (or none at all) even when it comes to build time.

@ciaranmcnulty ciaranmcnulty mentioned this pull request Apr 11, 2022
Closed
@ndeloof
Copy link
Collaborator

ndeloof commented Apr 11, 2022

different secrets makes sense to me (so the ability to provide overrides on compose file, or use paths relative to user's home ~/ to define a secret). I hardly understand how no secret would be used, as this would mean resource can be accessed without a secret? Maybe better discus ability to declare optional secrets in a separate proposal

@Kludex Kludex mentioned this pull request Apr 19, 2022
Closed
@EricHripko
Copy link
Collaborator

Sure, I agree that this can be discussed in a separate thread. Let me get some resources ready to see if there is a substantial justification for such functionality 👍

@GhandiJones
Copy link

Any update on this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@EricHripko EricHripko EricHripko left review comments

@ndeloof ndeloof ndeloof approved these changes

@ulyssessouza ulyssessouza ulyssessouza approved these changes

Assignees

@glours glours

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@glours @ndeloof @EricHripko @GhandiJones @ulyssessouza