Save PROJECT_SOURCE for containers mounting source#3979
Save PROJECT_SOURCE for containers mounting source#3979openshift-merge-robot merged 7 commits intoredhat-developer:masterfrom
Conversation
|
Skipping CI for Draft Pull Request. |
Codecov Report
@@ Coverage Diff @@
## master #3979 +/- ##
==========================================
+ Coverage 43.17% 43.29% +0.11%
==========================================
Files 146 147 +1
Lines 12338 12411 +73
==========================================
+ Hits 5327 5373 +46
- Misses 6445 6470 +25
- Partials 566 568 +2
Continue to review full report at Codecov.
|
01aa4a1 to
2d5afd5
Compare
|
@adisky @johnmcollier Could you pls help review the PR? thx! |
| Value: syncFolder, | ||
| }) | ||
| } else { | ||
| return nil, fmt.Errorf("env variable %s is reserved and cannot be customized in component %s", adaptersCommon.EnvProjectsSrc, comp.Name) |
There was a problem hiding this comment.
Do we need this if we also have https://github.com/openshift/odo/pull/3979/files#diff-ab356982336b3424cbe588733e4259ffR39?
There was a problem hiding this comment.
Yeah I thought about this. But I was wondering if for some reason, devfile parse validation changes in the future and it removed the env check; then we'd somehow allow this when creating a container. So put it as double fail safe. I know its redundant but i can remove it, if you feel strongly
There was a problem hiding this comment.
IMO, we just need the one check for now to avoid having duplicate code (and code that won't get triggered currently), and if anyone overhauls the validation code in the future, it's their responsibility to make sure the check for PROJECT_SOURCE still occurs
But I'm totally flexible either way, TBH. I can see where you're coming from.
There was a problem hiding this comment.
With devfile parser being extracted out soon, would we want to validate this at the parser level or push level? 🤔
There was a problem hiding this comment.
At the parser level in my opinion, so the validation should be kept there as well.
Modifying PROJECT_SOURCE isn't valid at the devfile spec level, so it makes sense to have the check there
There was a problem hiding this comment.
Opened devfile/api#132 for discussion too, FYI.
Your point about moving out the devfile parser reminded me that we don't really have anything in the spec right now that says PROJECT_SOURCE can't be overridden.
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>
Signed-off-by: Maysun J Faisal <maysunaneek@gmail.com>
60046ce to
072781e
Compare
Signed-off-by: Maysun J Faisal <maysunaneek@gmail.com>
|
/retest |
adisky
left a comment
There was a problem hiding this comment.
@maysunfaisal Tried it out working well, code wise also looks good.
/lgtm
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: girishramnani The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Signed-off-by: Maysun J Faisal maysunaneek@gmail.com
/kind feature
What does does this PR do / why we need it:
PROJECT_SOURCEto all component containers that mount sourcesPROJECT_SOURCEwhich holds the project path in a containerPROJECT_SOURCEvalue depends upon container'ssourceMapping& project'sclonePathGetSyncFolder()logic for devfile containing more than one project ie; the first project is now selectedPROJECTS_ROOT&PROJECT_SOURCEare reserved, check Should PROJECTS_ROOT and PROJECT_SOURCE be reserved environment variables? devfile/api#132Which issue(s) this PR fixes:
Fixes #3781
PR acceptance criteria:
Unit test
Integration test
Documentation
I have read the test guidelines
How to test changes / Special notes to the reviewer:
clonePathand container'ssourceMappingPROJECT_SOURCEcannot be explicitly mentioned in the devfile