fix: devspace deploy do git pull for cached dependencies#2766
fix: devspace deploy do git pull for cached dependencies#2766lizardruss merged 29 commits intodevspace-sh:mainfrom
Conversation
✅ Deploy Preview for devspace-docs ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site configuration. |
lizardruss
left a comment
There was a problem hiding this comment.
Thanks for the PR! I like where this is going.
I left some comments, but they may not be clear. More generally, I'd like to call repo.Clone() and repo.Pull() separately, so that it becomes clearer that we skip pulling when a revision has been specified.
I think this would also get rid of the if / else between clone and pull, and make it more like
- clone (if needed)
- pull (if no revision is specified and pulling is not disabled)
This might also make it possible to push the switchURL() logic into repo.Clone & repo.Pull, but I'm ok with duplicating it for logging purposes.
|
|
||
| // make sure the repo is up-to-date | ||
| // Make sure the repo is up-to-date | ||
| gr.Pull(ctx, options) |
There was a problem hiding this comment.
I think I might like to see this called after this line when no revision is specified.
This would allow removing the if options.Commit == "" check from Pull, which I think makes it more difficult to understand.
There was a problem hiding this comment.
Agreed, take a look
|
|
||
| log.Debugf("Cloned %s", gitPath) | ||
| } else if !source.DisablePull { | ||
| repo.Pull(ctx, gitCloneOptions) |
There was a problem hiding this comment.
If pulling is removed from repo.Clone, then we could change this from an else if to an if:
if !source.DisablePull && source.Revision == "" {
// Pull
}May need to also handle the error, and attempt to switching the URL, similar to this line.
There was a problem hiding this comment.
I agree, take a look again.
I am not sure if me messing up the commit history matters? I hope we can squash into the origin repository.
I retested
- It correctly git clone repos when i delete .dependencies folder
- It correctly reclone git repo when a branch is changed
- It correctly git pull cached dependencies
- It respects the dependency.disablePull flag by not pulling when its true
|
Will look at this again later this week |
|
ℹ️ Output from newest testing/code, looks good (with considerations within the scope of this PR). Nothing is cloned twice, the amount of pulls are as expected. |
|
I believe this is ready for merge @lizardruss (just remember to Squash) |
lizardruss
left a comment
There was a problem hiding this comment.
Thanks for the changes! LGTM! Will run tests and squash the commits when merging.
Signed-off-by: André S. Hansen <andre.ok@online.no>
Signed-off-by: André S. Hansen <andre.ok@online.no>
Signed-off-by: André S. Hansen <andre.ok@online.no>
Signed-off-by: Fabian Kramm <fab.kramm@googlemail.com> Signed-off-by: André S. Hansen <andre.ok@online.no>
Signed-off-by: Luca Di Maio <luca.dimaio1@gmail.com> Signed-off-by: André S. Hansen <andre.ok@online.no>
Signed-off-by: Luca Di Maio <luca.dimaio1@gmail.com> Signed-off-by: André S. Hansen <andre.ok@online.no>
This is a mitigation for devspace-sh#2448 (comment) Signed-off-by: Matthias Riegler <matthias.riegler@ankorstore.com> Signed-off-by: André S. Hansen <andre.ok@online.no>
Signed-off-by: xcming <326658575@qq.com> Signed-off-by: André S. Hansen <andre.ok@online.no>
Signed-off-by: Fabian Kramm <fab.kramm@googlemail.com> Signed-off-by: André S. Hansen <andre.ok@online.no>
Signed-off-by: André S. Hansen <andre.ok@online.no>
Test is currently failing due to devspace-sh#2415, so this is a temporary measure to allow unrelated PRs to be approved Signed-off-by: Russell Centanni <russell.centanni@gmail.com> Signed-off-by: André S. Hansen <andre.ok@online.no>
Bumps [follow-redirects](https://github.com/follow-redirects/follow-redirects) from 1.15.2 to 1.15.4. - [Release notes](https://github.com/follow-redirects/follow-redirects/releases) - [Commits](follow-redirects/follow-redirects@v1.15.2...v1.15.4) --- updated-dependencies: - dependency-name: follow-redirects dependency-type: indirect ... Signed-off-by: dependabot[bot] <support@github.com> Signed-off-by: André S. Hansen <andre.ok@online.no>
Bumps [follow-redirects](https://github.com/follow-redirects/follow-redirects) from 1.14.8 to 1.15.4. - [Release notes](https://github.com/follow-redirects/follow-redirects/releases) - [Commits](follow-redirects/follow-redirects@v1.14.8...v1.15.4) --- updated-dependencies: - dependency-name: follow-redirects dependency-type: indirect ... Signed-off-by: dependabot[bot] <support@github.com> Signed-off-by: André S. Hansen <andre.ok@online.no>
Signed-off-by: Maarten Deprez <mdp@si-int.eu> Signed-off-by: André S. Hansen <andre.ok@online.no>
…od, to fix endless wait on container startup with multiple containers and startContainer enabled Signed-off-by: Maarten Deprez <mdp@si-int.eu> Signed-off-by: André S. Hansen <andre.ok@online.no>
Signed-off-by: Maarten Deprez <mdp@si-int.eu> Signed-off-by: André S. Hansen <andre.ok@online.no>
…mand Signed-off-by: Russell Centanni <russell.centanni@gmail.com> Signed-off-by: André S. Hansen <andre.ok@online.no>
Signed-off-by: Limiardi Eka Sancerio <limiardi.sancerio@zendesk.com> Signed-off-by: André S. Hansen <andre.ok@online.no>
Signed-off-by: Maarten Deprez <mdp@si-int.eu> Signed-off-by: André S. Hansen <andre.ok@online.no>
Signed-off-by: Luca Di Maio <luca.dimaio1@gmail.com> Signed-off-by: André S. Hansen <andre.ok@online.no>
Signed-off-by: Luca Di Maio <luca.dimaio1@gmail.com>
Signed-off-by: Fabian Kramm <fab.kramm@googlemail.com>
Test is currently failing due to devspace-sh#2415, so this is a temporary measure to allow unrelated PRs to be approved Signed-off-by: Russell Centanni <russell.centanni@gmail.com> Signed-off-by: André S. Hansen <andre.ok@online.no>
Signed-off-by: Maarten Deprez <mdp@si-int.eu>
Signed-off-by: Maarten Deprez <mdp@si-int.eu> Signed-off-by: André S. Hansen <andre.ok@online.no>
…mand Signed-off-by: Russell Centanni <russell.centanni@gmail.com> Signed-off-by: André S. Hansen <andre.ok@online.no>
Signed-off-by: Limiardi Eka Sancerio <limiardi.sancerio@zendesk.com> Signed-off-by: André S. Hansen <andre.ok@online.no>
Signed-off-by: André S. Hansen <andre.ok@online.no>
|
@lizardruss - Fixed lint errors, at least i think i have, i was not able to reproduce them locally, and the workflow here is not run until you trigger them. |
|
@lizardruss - Any chance this could be merged? |
What issue type does this pull request address?
/kind bugfix
/kind enhancement
What does this pull request do? Which issues does it resolve?
resolves #2765
Please provide a short message that should be published in the DevSpace release notes
Same as PR title
What else do we need to know?
Alternatively the PR #2709 could be straight up reverted. As reverting would be (i think) functionally the same as this PR. However, it does not read very well. It was not a coincidence that i misunderstood the "git clone" code path of DownloadDependency that i was attempting to fix in my last pr, thought, I am no go developer and are in that sense reading and writing code with a handicap. :P
Testing
As I am not able to write integration tests, i have manually tested a locally built devspace.exe: