Request -> Config refactor + additional cleanups. #204
Conversation
| // InstallDestination allows to override the default destination of the STI | ||
| // scripts. It allows to place the scripts into application root directory | ||
| // (see ONBUILD strategy). The default value is "upload/scripts". | ||
| InstallDestination string |
There was a problem hiding this comment.
pkg/build/strategies/sti/sti.go
71: inst := script.NewInstaller(req.BaseImage, req.ScriptsURL, req.InstallDestination, docker)This was originally used here, for some reason it is not anymore. Is there a reason why?
There was a problem hiding this comment.
It's not being used anymore from what I looked into. And the problem this variable was trying to address is solved differently from what I tested. Currently this function copies STI scripts to main app dir which works nicely. If there are other usecases that this was addressing we don't have them written down in any reproducable way (eg. integration tests). That's why I've also opened #203 to track these.
|
@soltysh few nits but I really like this refactoring. Good job! |
| } | ||
| } | ||
|
|
||
| if !validRequest(req) { | ||
| if len(validation.ValidateConfig(cfg)) != 0 { |
There was a problem hiding this comment.
I think it'd be ideal if we moved some of this logic to a function in a different package (anything that doesn't have to do with the environment or command line args) so we can reuse it from origin. Otherwise we'd have to repeat the same code in origin (validating the config, creating the build strategy, etc)
There was a problem hiding this comment.
That was the initial goal, to have validation method available to be run in origin (see #144). But I agree we can revisit this method once again to do that. I've created an issue to track that #206. If you don't mind, I'll do that as a follow-up. I don't want to get this PR more hairy than it's already is 😉
|
Apart from comments above, LGTM |
|
@csrwng since that last comment was on commit and it's lost, I've added additional validation for DockerConfig.Endpoint, as this what we use in origin, at least now and it makes sense to have required. |
Reviewed and removed unnecessary fields from Config, which was also renamed from Request (fixed openshift#100).
|
LGTM |
|
[merge] |
|
Source To Image Merge Results: SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_requests_sti/109/) |
|
Evaluated for source to image up to ad4dd9e |
Merged by openshift-bot
I've added method for validating Config object - should fix #144. I've also reviewed and removed unnecessary fields from Config, which was also renamed from Request (fixed #100).
@bparees @mfojtik @csrwng PTAL, that's the last refactor I'd like to make before updating the origin's version. Maybe we could name it 1.0 release? Any thoughts one that are welcome as well.