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

RFC: Process specific env and profile.d #83

Merged
merged 1 commit into from
Jun 10, 2020

Conversation

jabrown85
Copy link
Contributor

@jabrown85 jabrown85 commented May 30, 2020

Readable

Derived from #72

@hone
Copy link
Member

hone commented Jun 1, 2020

thanks for picking this up.

@sclevine sclevine self-requested a review June 2, 2020 21:14
@nebhale nebhale self-requested a review June 3, 2020 17:20
@nebhale
Copy link
Contributor

nebhale commented Jun 3, 2020

Final Comment Period with merge disposition, closing on 10 June, 2020.

[unresolved-questions]: #unresolved-questions

- Should this idea be extended to `bin`, `lib`, and any other buildpack output intended for launch?
- Should lifecycle ignore invalid entries like `<layers>/<layer>/env.build/web/NODE_OPTIONS`?
Copy link
Member

@hone hone Jun 3, 2020

Choose a reason for hiding this comment

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

I wonder if it should just error instead of silently ignoring it.

# Unresolved Questions
[unresolved-questions]: #unresolved-questions

- Should this idea be extended to `bin`, `lib`, and any other buildpack output intended for launch?
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure what the use case is? @jabrown85

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A user might want to add a credential helper binary for web but not have that accessible for debug. They could put the binary at bin/web/credential-helper to achieve this if we decided to allow this.

Copy link
Member

Choose a reason for hiding this comment

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

Is that a different image at that point?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure. I think it's reasonable to think that perhaps a web process is restricted to having access to something in bin/lib that perhaps you wouldn't want to be immediately accessible for non-defined launcher executions /cnb/lifecycle/launcher -- bash for instance.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I guess without changing the image it's still technically there. I'm happy to mark this as out of scope of the RFC unless you feel strongly about it.

Copy link
Member

@hone hone left a comment

Choose a reason for hiding this comment

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

I'm 👍 on this RFC with the additional use case identified, but I've requested some changes and clarifications.

text/0000-process-specific-env.md Outdated Show resolved Hide resolved
text/0000-process-specific-env.md Outdated Show resolved Hide resolved
- Should lifecycle ignore invalid entries like `<layers>/<layer>/env.build/web/NODE_OPTIONS`?

# Spec. Changes (OPTIONAL)
[spec-changes]: #spec-changes
Copy link
Member

Choose a reason for hiding this comment

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

Can you detail out some of the spec changes 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.

Done. I bolded the parts I think need to change.

Copy link
Member

Choose a reason for hiding this comment

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

@ekcasey @sclevine can you review this?

Signed-off-by: Jesse Brown <jabrown85@gmail.com>
nebhale added a commit that referenced this pull request Jun 10, 2020
[#83]

Signed-off-by: Ben Hale <bhale@vmware.com>
@nebhale nebhale merged commit 51c3f78 into buildpacks:master Jun 10, 2020
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.

None yet

7 participants