-
Notifications
You must be signed in to change notification settings - Fork 7
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
[Do not merge] For testing #35
Conversation
Codecov Report
@@ Coverage Diff @@
## master #35 +/- ##
==========================================
- Coverage 88.93% 88.72% -0.21%
==========================================
Files 13 13
Lines 795 798 +3
==========================================
+ Hits 707 708 +1
- Misses 88 90 +2
Continue to review full report at Codecov.
|
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 the change? We already handle this... see inline.
@@ -287,6 +288,11 @@ class OpenwhiskActionRunner { | |||
_getImage() { | |||
// blackbox -> image is specified directly | |||
if (this.action.exec.image) { | |||
if(process.env.ASSET_COMPUTE_DOCKER_IMAGE_PREFIX |
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.
(Iff we need that var)
I would name this ADOBE_RUNTIME_DOCKER_REGISTRY
(it really is the registry not the prefix, the prefix would be nui/
) and use this name consistently everywhere including our CI: https://git.corp.adobe.com/nui/api-process/pull/81#discussion_r3122910
cc @pheenomenon
if(process.env.ASSET_COMPUTE_DOCKER_IMAGE_PREFIX | ||
&& validUrl.isHttpsUri(process.env.ASSET_COMPUTE_DOCKER_IMAGE_PREFIX)){ | ||
// for blackbox containers that may grab images from repositories depending on region | ||
return process.env.ASSET_COMPUTE_DOCKER_IMAGE_PREFIX + this.action.exec.image; |
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.
I am confused - we already handle this here in the nui cli.
What we currently do:
- all workers have the docker registry in their image name in package.json
- this works for local wskdebug & test-worker (code here)
- upon deployment, it gets removed as Runtime no longer accepts a registry in there anymore (since July 2020)
Based on that, why is this change even required?
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.
Doing it the other way around (no prefix in sources, only add for local testing) is tricky as it wouldn't work for the generic wskdebug
which needs to pull the image as well but has no idea what registry different images might use.
Actually, I think those changes are not needed. At least not for initial deployment. Will reopen and make requested changes if needed/as needed. |
Use a prefix for docker image name to set image source depending on region.