-
Notifications
You must be signed in to change notification settings - Fork 243
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
Save PROJECT_SOURCE for containers mounting source #3979
Save PROJECT_SOURCE for containers mounting source #3979
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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
Needs 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_SOURCE
to all component containers that mount sourcesPROJECT_SOURCE
which holds the project path in a containerPROJECT_SOURCE
value depends upon container'ssourceMapping
& project'sclonePath
GetSyncFolder()
logic for devfile containing more than one project ie; the first project is now selectedPROJECTS_ROOT
&PROJECT_SOURCE
are 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:
clonePath
and container'ssourceMapping
PROJECT_SOURCE
cannot be explicitly mentioned in the devfile