-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
feat: kaniko dir config option #1997
feat: kaniko dir config option #1997
Conversation
@imjasonh I can't set you as a reviewer myself unfortunately 😢 Could you approve the workflows too, please 😄 |
This looks good! We'll see if the tests pass, and I think we can merge this. 👍 But it's not the whole story either. If a build has |
Much appreciated @imjasonh Looks as though it had an issue with the credentials on the PR itself, Is this what you mean by the cred helpers not being in the correct place? Unsure whether it's a temporary network issue too on the GitHub Actions side, I know they've had a couple of problems recently, with it being a TLS timeout, I'll take a dig into it a little bit later, just to be absolutely sure. |
That TLS issue seems like a flake, I'll just keep retrying until that works. Re: cred helpers etc. The Kaniko image contains all its own important bits in the Lines 52 to 58 in 8651c06
As the executor runs, it fills up its own filesystem with the results of the ongoing build, and after each With your change, all of this should work just fine -- if So what we need to do -- it doesn't have to be in this PR, but we should plan to do it soon -- is, at startup, if Does that make sense? It's all pretty mind-bending. Let me know if I can help explain it better in any way. |
I believe that makes sense, so that change (I'll raise another PR later on) would be specifically targeted towards the files within I had a skim over the Do you think it might be best served to have another issue for it as well? Feel free to assign me onto that as well as I'll link back to your comment for reference in the resulting PR, so others can see a full conversation history as it evolved. |
Files inside The change we need is for the executor to understand that if it's not in That might actually just be as easy as adding
All of this is part of the overarching work to make |
That makes a lot of sense, I was getting confused in thinking we'd need to modify the actual image build process itself. I've added a small check now. The unit tests are passing locally from I believe there's certainly some extra test cases to be done. I'd be hesitant to test the |
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.
This looks promising! I'll try to figure out how to run a test where KANIKO_DIR
is set to something else, to make sure this works. Thanks for working on this!
Co-authored-by: Jason Hall <[email protected]>
No worries at all Jason, much appreciated for your help too! |
Looking at the integration tests it looks like it should be fairly straightforward to add one that exercises kaniko/integration/integration_test.go Line 338 in 2a8c565
You could do something like:
You can run that with If that build works, then it tell us that setting |
a77ff5d
to
355ae4a
Compare
That's super useful, much appreciated Jason. I've fixed the If you don't mind, I'd like to quickly sanity check with the CI, as I'm seeing a failure, even on the integration_test.go:414: []integration.diffOutput differ (-got, +want): []integration.diffOutput{
{
Image1: "localhost:5000/docker-dockerfile_registry_mirror",
Image2: "localhost:5000/kaniko-dockerfile_registry_mirror",
DiffType: "File",
Diff: &integration.fileDiffResult{
Adds: nil,
- Dels: []integration.fileDiff{{Name: "/etc/machine-id", Size: 33}},
+ Dels: nil,
},
} For some reason, the |
That's definitely weird! It doesn't seem related to your change though, so I wouldn't worry about it for now. |
The tests passed in the CI, so it looks like it was some weird configuration on my end - I'm developing on a VM, so I wonder whether there's any funkiness going on there with |
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.
This looks really good! The integration test especially makes me pretty confident this isn't going to break any existing behavior. 👍
Co-authored-by: Jason Hall <[email protected]>
Co-authored-by: Jason Hall <[email protected]>
Co-authored-by: Jason Hall <[email protected]>
Yeah absolutely, the extra test makes me less anxious about it 😆 Much appreciated for pointing me in the right direction on the example test to follow! I've added your suggested changes now too, no worries on the styling side. I think I'll actually start implementing what you said in my own stuff, I quite like it 😄 |
@jdockerty I'm very excited about this new functionality but I'm seeing issues with this PR as others are mentioning in #1945.
I made a quick playground repro of this issue here https://play.golang.com/p/ev1FjrASYAp Any ideas on how to handle this? Maybe if the destination doesn't exist, the rename is easy and will succeed as long as we don't create it. If the destination does exist, I think we may need to recursively copy the contents of /kaniko there. |
@shpprfb I do believe I know where the issue is of it not picking it up, it was an oversight by me. Here it is passing the Thanks for raising this 😄 |
Fixes #1945
Description
Moving away from a hard-coded
/kaniko
directory, this was inconstants.go
, to one which is configurable by the user - this can be done either through a flag,--kaniko-dir
, or an environment variable,KANIKO_DIR
.Submitter Checklist
These are the criteria that every PR should meet, please check them off as you
review them:
See the contribution guide for more details.
Reviewer Notes
Release Notes
executor
adds a new flag--kaniko-dir
or environment variableKANIKO_DIR
to override the default directory that kaniko uses.