Explicitly state PROJECTS_ROOT & PROJECT_SOURCE are reserved#134
Explicitly state PROJECTS_ROOT & PROJECT_SOURCE are reserved#134maysunfaisal merged 6 commits intodevfile:masterfrom
Conversation
elsony
left a comment
There was a problem hiding this comment.
The current approach is to repeat the same info on places that have access to those two variables. Instead of repeating the same info all over the place, will it be cleaner to specify under the container env section and mentioned those two are reserved variable and cannot be overridden? In reality, the user can only override those variables in the container's env setting so it is probably better to put that restriction right on that section.
|
I have generic comment, is it necessary to add the full definitions for |
|
Yeah, I agree with @elsony, I think we just need a section in the container.env schema that outlines which variables are reserved and can't be set by the user. Much like how we outline allowed env variables in the And I like @GeekArthur's suggestion, it would be nice if we could define all of the relevant environment variables in the devfile spec in one spot, rather than having to define them every time we mention them in the spec. Especially if we add more reserved environment variables to the schema in the future, it'll keep the schema cleaner. But TBH, that's outside of the scope of this PR. |
|
@elsony @GeekArthur @johnmcollier updated the PR! Makes sense. EDIT - I moved info regd the env to the env section and just mentioned that these env can be used in the command. Lmk, if it looks cleaner now |
|
@maysunfaisal It looks cleaner to me! I didn't find a PR review guide for this repo, probably we should let the code owner do the final review. |
|
@davidfestal @amisevsk @sleshchenko Can I pls get a review on this? Thx! |
| // - `$PROJECTS_ROOT`: A path where projects sources are mounted | ||
| // - `$PROJECTS_ROOT` | ||
| // | ||
| // - `$PROJECT_SOURCE`: A path to a project source ($PROJECTS_ROOT/<project-name>). If there are multiple projects, this will point to the directory of the first one. | ||
| // - `$PROJECT_SOURCE` |
There was a problem hiding this comment.
I'm not sure about removing the descriptions of the variables here is a good idea (at least, without some other, centralized location we can link to to explain it). Currently, the only way to figure out what $PROJECTS_ROOT and $PROJECT_SOURCE are to be used for is by checking the description for env, where the main purpose is to tell you that you can't override them. Basically, my thinking works out to:
ExecCommand(the use-site): Describe what the variables can be used forContainer(the define-site): Describe which variables can be overridden (or not)
An alternative if we want to keep the command descriptions terse is to e.g. link to a docs page (once a description of reserved env vars is added -- e.g. here).
Alternatively, a more generic doc would remove the "Special variables..." entirely; isn't it the case that you can use any environment variable in a command? The only difference for $PROJECTS_ROOT and $PROJECT_SOURCE is that they're supposed to automatically be set, but you should be able to reference e.g. $M2_HOME, etc. here as well.
// The actual command-line-string. Note environment variables from the container the command runs
// in can be used in the command, e.g.
//
// `cd ${PROJECT_SOURCE} && mvn clean install`
I'm less certain this applies to the workingDir, where I think documenting the automatically-defined env vars is useful.
There was a problem hiding this comment.
That makes sense. I've reverted the changes and just kept the section about reservation in env.
I've opened #138 to discuss and track on how we can define special variables in devfile spec context, as that seems to be out of this PR's scope.
Signed-off-by: Maysun J Faisal <maysunaneek@gmail.com>
Signed-off-by: Maysun J Faisal <maysunaneek@gmail.com>
Signed-off-by: Maysun J Faisal <maysunaneek@gmail.com>
Signed-off-by: Maysun J Faisal <maysunaneek@gmail.com>
Signed-off-by: Maysun J Faisal <maysunaneek@gmail.com>
5e7fdc2 to
e1a80ff
Compare
|
I've rebased and updated the PR! |
| \n - `$PROJECT_SOURCE`: A path to a project source | ||
| ($PROJECTS_ROOT/<project-name>). If there are multiple | ||
| projects, this will point to the directory of the first | ||
| one" |
| A path to a project source ($PROJECTS_ROOT/<project-name>). | ||
| If there are multiple projects, this will point to the | ||
| directory of the first one." | ||
| directory of the first one" |
There was a problem hiding this comment.
The reason I removed the ending period is because I thought a bullet point should not necessarily have an ending period and the schema was inconsistent in this way. Some had and some didnt. So I removed the periods from the struct i was working on
There was a problem hiding this comment.
I've checked online and the general rule is; complete sentences should have periods and fragmented sentences should not have periods. I've updated the PR accordingly.
| A path to a project source ($PROJECTS_ROOT/<project-name>). | ||
| If there are multiple projects, this will | ||
| point to the directory of the first one." | ||
| point to the directory of the first one" |
| \ - `$PROJECT_SOURCE`: A path to a project | ||
| source ($PROJECTS_ROOT/<project-name>). If | ||
| there are multiple projects, this will point | ||
| to the directory of the first one" |
| A path to a project source ($PROJECTS_ROOT/<project-name>). | ||
| If there are multiple projects, this will point | ||
| to the directory of the first one." | ||
| to the directory of the first one" |
| a project source ($PROJECTS_ROOT/<project-name>). | ||
| If there are multiple projects, this will point | ||
| to the directory of the first one." | ||
| to the directory of the first one" |
| source ($PROJECTS_ROOT/<project-name>). | ||
| If there are multiple projects, this will | ||
| point to the directory of the first one." | ||
| point to the directory of the first one" |
| sourceMapping \n - `$PROJECT_SOURCE`: A path to a project | ||
| source ($PROJECTS_ROOT/<project-name>). If there are multiple | ||
| projects, this will point to the directory of the first | ||
| one" |
There was a problem hiding this comment.
Missing ending period. The same for the rest so just do a search and I am not marking the others.
Signed-off-by: Maysun J Faisal <maysunaneek@gmail.com>
Signed-off-by: Maysun J Faisal maysunaneek@gmail.com
What does this PR do?
This schema explicitly specifies that
PROJECTS_ROOT&PROJECT_SOURCEenv are reserved and cannot be overridden via component container's envWhat issues does this PR fix or reference?
Fixes #132
Is your PR tested? Consider putting some instruction how to test your changes
Docs PR