Resolves #1124: Always scan the repository for provided scripts #1125
Conversation
Whether the base image provides the scripts or not, the repository can supply override scripts. Removes the expectation/need of a location specified in the image.
|
i think we're lacking some fundamental clarity about the precedence ordering of scripts. i think it should be:
The existing code, prior to this PR, does not prioritize scripts in the repository. So in that sense this is a good change. However the change also now prioritizes scripts from the repo over whatever config/args the user might have provided at runtime to indicate where they want to get their scripts from, meaning that you have to delete the scripts from your repository to be able to use an alternate location. Which potentially changes the behavior for existing users who are passing tldr: i think this needs a little more thought/study to straighten out the interactions of the various ways of providing scripts. The other subtly here is that there's an intent to union together the sources of scripts (e.g. you might get your assemble script from the local repo, but the run script might still come from the builder image). Looking at this code(both the original and the patch) i'm not sure how all that comes together at the moment. we also need to compare this behavior with the behavior when we directly build the image (the code path being modified here is only for the "create a dockerfile" mode, not the "invoke docker build mode" which is somewhat different) and ensure we're being consistent in how scripts are chosen between the two modes. |
In my s2i command I was using to test this, I didn't specify any command line args like that (only had the annotation in the image and the stuff in the local repo). I didn't see where these would be distinguishable in the fragment of code I was looking at; it must be further upstream where that is lost. I'll get back to this if somebody else doesn't first. /hold if we can (I'm looking for the WIP toggle too) |
openshift-ci
bot
added
the
do-not-merge/hold
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
/test all |
Whether the base image provides the scripts or not, the repository can supply override scripts. Removes the expectation/need of a location specified in the image.
|
/hold cancel |
openshift-ci
bot
removed
the
do-not-merge/hold
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
|
Have reworked this PR to honor the documented ordering and pick up the .s2i scripts in the local repository in the right spot. Re-ran my checks and saw the right behaviors. I noticed that annotations or command line parameters which might use http:// or file:// aren't really considered/used in the dockerfile.go (they could be checked/fetched and merged into the resolved list), but I didn't try to resolve any of that. |
|
/assign |
|
/assign @bparees |
i'd prefer you internalize the summary i gave of how this is supposed to work and assess whether this PR achieves that behavior (or ask me questions about my summary if you think it's incorrect or unclear in the expectations it sets). |
|
@cuppett: The following test failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
| } | ||
| if strings.HasPrefix(config.ImageScriptsURL, "image://") { | ||
| return strings.TrimPrefix(config.ImageScriptsURL, "image://"), false | ||
| return strings.TrimPrefix(config.ImageScriptsURL, "image://"), providedScripts |
There was a problem hiding this comment.
Shouldn't this return make(map[string]bool) instead according to it's previous use?
There was a problem hiding this comment.
I don't think so. This function has been expanded to merge the precedence order (which was the issue with the previous version).
In this case, when scripts have been found in the local repo, but then in the 3rd and 4th priority cases where an annotation or fallback exists, we should merge/override with the scripts from the local repo.
The only correct spot to return an empty found map is when we have a command line parameter pointing to image:/// location (because we won't know and don't inspect the image to see which scripts may be there, so it's unilateral).
It'd be worthwhile in a future enhancement to understand more deeply how we should handle or inspect when file:/// or http:// are indicated for these ordered search locations. I couldn't see where that it's inspected anywhere and didn't try it.
| }, | ||
| } | ||
| for _, tc := range cases { | ||
| output, hasScripts := getImageScriptsDir(tc.Config) | ||
| output, _ := getImageScriptsDir(tc.Config, &(Dockerfile{})) |
There was a problem hiding this comment.
Shouldn't some of these tests check for the returned map and it's contents (or lack thereof) since the hasScripts bool is being changed?
There was a problem hiding this comment.
The tests here were pretty deficient from what I saw. It only tested a "true" condition and empty map without providing any file-based overrides.
I could revise this to plop &(Dockerfile{}) in the stanzas like the "true" previously; however, it would just confuse reviewers again next time that there is an actual validation here (versus it being static/empty in all cases). I think this representation more accurately matches the current and previous value of what the test was providing us.
I looked briefly at how to mock filesystem entries but I think I'll make a bigger mess before I provide more value.
|
/lgtm |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: coreydaley, cuppett 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 |
1 similar comment
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: coreydaley, cuppett 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 |
openshift-ci
bot
added
the
approved
Whether the base image provides the scripts or not, the repository can supply override scripts. Removes the expectation/need of a location specified in the image.