Skip to content
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

#296 Support remote helm chart repositories #1254

Merged
merged 6 commits into from Jan 30, 2019

Conversation

eraac
Copy link
Contributor

@eraac eraac commented Nov 8, 2018

Fix #296 based on this #296 (comment)

I added the possibility to skip build dependencies

make test pass, but i have weird error after, maybe someone have an idea ?

PASS
coverage: 94.0% of statements
ok      github.com/GoogleContainerTools/skaffold/pkg/skaffold/yamltags  0.008s  coverage: 94.0% of statements
?       github.com/GoogleContainerTools/skaffold/pkg/webhook/constants  [no test files]
?       github.com/GoogleContainerTools/skaffold/pkg/webhook/github     [no test files]
?       github.com/GoogleContainerTools/skaffold/pkg/webhook/kubernetes [no test files]
?       github.com/GoogleContainerTools/skaffold/pkg/webhook/labels     [no test files]
?       github.com/GoogleContainerTools/skaffold/testutil       [no test files]
?       github.com/GoogleContainerTools/skaffold/webhook        [no test files]
Running validation scripts...
RUN hack/boilerplate.sh
PASSED hack/boilerplate.sh
RUN hack/gofmt.sh
PASSED hack/gofmt.sh
RUN hack/linter.sh
Installing GolangCI-Lint
golangci/golangci-lint info checking GitHub for tag 'v1.9.3'
golangci/golangci-lint info found version: 1.9.3 for v1.9.3/darwin/amd64
golangci/golangci-lint info installed /Users/kevin/Code/go/bin/golangci-lint
WARN [runner/megacheck] Can't run megacheck because of compilation errors in packages [runtime/internal/atomic runtime internal/cpu runtime/internal/sys]: ../../../../../../../../usr/local/Cellar/go/1.11.1/libexec/src/runtime/internal/atomic/atomic_wasm.go:14: Load redeclared in this block and 67 more errors: run `golangci-lint run --no-config --disable-all -E typecheck` to see all errors 
pkg/skaffold/schema/latest/config.go:183:18: struct of size 160 bytes could be of size 152 bytes (maligned)
type HelmRelease struct {
                 ^
...

I have change HelmRelease for implementing this, so it's probably link to this error

Copy link
Contributor

@r2d4 r2d4 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Put the new bool field next to the other bools

@eraac
Copy link
Contributor Author

eraac commented Nov 8, 2018

@r2d4 fixed :) any explanation?

@codecov-io
Copy link

codecov-io commented Nov 8, 2018

Codecov Report

Merging #1254 into master will increase coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1254      +/-   ##
==========================================
+ Coverage   45.62%   45.63%   +0.01%     
==========================================
  Files         116      116              
  Lines        4870     4871       +1     
==========================================
+ Hits         2222     2223       +1     
  Misses       2424     2424              
  Partials      224      224
Impacted Files Coverage Δ
pkg/skaffold/deploy/helm.go 64.84% <100%> (+0.16%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a190f60...d99508c. Read the comment docs.

@r2d4
Copy link
Contributor

r2d4 commented Nov 8, 2018

Memory gets allocated in 8 byte blocks
Bool takes 1 byte

First grouping of 2 bools had 6 free bytes,
Second grouping of 1 had 7 free bytes

If you move all three together you free up the last block (8 bytes) and use one of the unused spaces!

Kind of a useless linter for skaffold however :)

@balopat balopat added the kokoro:run runs the kokoro jobs on a PR label Nov 8, 2018
@kokoro-team kokoro-team removed the kokoro:run runs the kokoro jobs on a PR label Nov 8, 2018
@eraac
Copy link
Contributor Author

eraac commented Nov 21, 2018

maybe the option could have a better name, like remoteChart, allowing to use it elsewhere and turn off or on some feature if needed.

waiting for your feedback

Copy link
Contributor

@balopat balopat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. minor nit around documentation.
  2. the double negation is a bit hard to read but I can be convinced
  3. would you mind writing unit tests around this behavior covering cases of null/true/false?

pkg/skaffold/schema/latest/config.go Show resolved Hide resolved
@eraac
Copy link
Contributor Author

eraac commented Nov 27, 2018

  1. change done for integration/examples/annoted-skaffold.yaml
  2. can be buildDependencies, but should be true by default and I have no idea how to do that
  3. job done for true/false, this value can't be null (I think so)

EDIT: unit test not pass on travis, i looking on it
EDIT2: fixed, but not sure to understand this line https://github.com/GoogleContainerTools/skaffold/pull/1254/files#diff-a1535a4f321ed1903fdc7f27ac166432R301 it's a correct value?

@balopat balopat added the kokoro:run runs the kokoro jobs on a PR label Dec 7, 2018
@kokoro-team kokoro-team removed the kokoro:run runs the kokoro jobs on a PR label Dec 7, 2018
@eraac
Copy link
Contributor Author

eraac commented Dec 21, 2018

@balopat can be the same issue here? #1408 (comment)

@eraac
Copy link
Contributor Author

eraac commented Jan 22, 2019

Apparently someone has done the work too ... #1368

@dgageot dgageot added the kokoro:run runs the kokoro jobs on a PR label Jan 28, 2019
@faheem-nadeem
Copy link

@eraac Any luck getting this figured out or do you need any support on this? Would love to see this merged :)

@kokoro-team kokoro-team removed the kokoro:run runs the kokoro jobs on a PR label Jan 28, 2019
@eraac
Copy link
Contributor Author

eraac commented Jan 30, 2019

I am very unlucky with kokoro ...

Get:34 http://archive.ubuntu.com/ubuntu xenial-backports/universe Translation-en [4184 B]
Fetched 18.3 MB in 4s (4046 kB/s)
Reading package lists...
E: Failed to fetch http://storage.googleapis.com/bazel-apt/dists/stable/jdk1.8/binary-amd64/Packages.gz  Hash Sum mismatch
E: Some index files failed to download. They have been ignored, or old ones used instead.
The command '/bin/sh -c apt-get update   && apt-get install -y bazel &&   rm -rf /var/lib/apt/lists/*' returned a non-zero code: 100
make: *** [integration-in-docker] Error 100
[ID: 1728516] Build finished after 71 secs, exit value: 2

@balopat balopat merged commit bfad453 into GoogleContainerTools:master Jan 30, 2019
@skhaz
Copy link

skhaz commented Feb 27, 2020

Still not working. See #3749

@skhaz
Copy link

skhaz commented Feb 27, 2020 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support remote helm chart repositories
9 participants