Skip to content

Implement checksum computation for sha256 tagger (fix issue #2290)#2301

Closed
seblegall wants to merge 1 commit intoGoogleContainerTools:masterfrom
seblegall:fix/issue-#2290-tagger-sha256
Closed

Implement checksum computation for sha256 tagger (fix issue #2290)#2301
seblegall wants to merge 1 commit intoGoogleContainerTools:masterfrom
seblegall:fix/issue-#2290-tagger-sha256

Conversation

@seblegall
Copy link
Copy Markdown
Contributor

@seblegall seblegall commented Jun 20, 2019

fix #2290

Instead of returning a latest tag by default the tagger will now
return a checksum based on the sha256 of the working dir.

The function walk across all files and directories of the working dir,
sum the sha256 of each and compute a checksum of the resulting sum.

Errors on some files only are ignored. If the directory cannot be walk
at all, then the default tag is latest.

@seblegall seblegall changed the title Fix(#2290): implement checksum computation for sha256 tagger Implement checksum computation for sha256 tagger (fix issue #2290) Jun 20, 2019
@seblegall seblegall force-pushed the fix/issue-#2290-tagger-sha256 branch 5 times, most recently from 459ac56 to 644f604 Compare June 20, 2019 16:13
@codecov
Copy link
Copy Markdown

codecov Bot commented Jun 20, 2019

Codecov Report

Merging #2301 into master will increase coverage by <.01%.
The diff coverage is 63.63%.

Impacted Files Coverage Δ
pkg/skaffold/build/tag/sha256.go 64.51% <63.63%> (-5.49%) ⬇️

@seblegall seblegall force-pushed the fix/issue-#2290-tagger-sha256 branch from 644f604 to 43ffe9d Compare June 20, 2019 16:52
@nkubala
Copy link
Copy Markdown
Contributor

nkubala commented Jun 20, 2019

@seblegall thanks for taking a stab at this one! looks like some of your tests are failing, can you take a look?

@dgageot
Copy link
Copy Markdown
Contributor

dgageot commented Jun 20, 2019

I don’t think there’s an issue to be fixed here. I’ll try to elaborate tomorrow when I have more time!

@seblegall
Copy link
Copy Markdown
Contributor Author

@nkubala Yes, I'm on it. Seems I have troubles with the temp file creation tools. I'm not sure why it fails but will have a look.

@dgageot I'm not sure it works. From what I can see, here is what appends :

func doDev(ctx context.Context, out io.Writer) error {
//...
		default:
			r, config, err := newRunner(opts)
			if err != nil {
				return errors.Wrap(err, "creating runner")
			}

			err = r.Dev(ctx, out, config.Build.Artifacts)

Then..

// First run
	if err := r.buildTestDeploy(ctx, out, artifacts); err != nil {

and finally..

// imageTags generates tags for a list of artifacts
func (r *SkaffoldRunner) imageTags(ctx context.Context, out io.Writer, artifacts []*latest.Artifact) (tag.ImageTags, error) {
//...
	for i := range artifacts {
		tagErrs[i] = make(chan tagErr, 1)

		i := i
		go func() {
			tag, err := r.Tagger.GenerateFullyQualifiedImageName(artifacts[i].Workspace, artifacts[i].ImageName)
			tagErrs[i] <- tagErr{tag: tag, err: err}
		}()
	}

So, In my understanding, if the image name doesn't already contains a tag, there is no reason one is added anywhere else.

@seblegall seblegall force-pushed the fix/issue-#2290-tagger-sha256 branch from 43ffe9d to 8e1fffc Compare June 21, 2019 09:27
@seblegall
Copy link
Copy Markdown
Contributor Author

Oh, and now that I just get more familiar with integration test, I found out that : https://github.com/GoogleContainerTools/skaffold/blob/master/integration/build_test.go#L65

Which seems to prouve expected tag for sha256 is always latest

@seblegall seblegall force-pushed the fix/issue-#2290-tagger-sha256 branch from 8e1fffc to d099ab4 Compare June 21, 2019 11:52
…a256 tagger

Instead of returning a "latest" tag by default the tagger will now
return a checksum based on the sha256 of the working dir.

The function walk across all files and directories of the working dir,
sum the sha256 of each and compute a checksum of the resulting sum.

Errors on some files only are ignored. If the directory cannot be walk
at all, then the default tag is latest.
@seblegall seblegall force-pushed the fix/issue-#2290-tagger-sha256 branch from d099ab4 to 5245cbe Compare June 21, 2019 12:12
@seblegall
Copy link
Copy Markdown
Contributor Author

@nkubala tests are now ok. It was a little tricky to make them fully reproducible. Let met know if you have any though about this PR. (logic, tests, etc..). 😃

@dgageot dgageot added the kokoro:run runs the kokoro jobs on a PR label Jul 2, 2019
@kokoro-team kokoro-team removed the kokoro:run runs the kokoro jobs on a PR label Jul 2, 2019
@nkubala
Copy link
Copy Markdown
Contributor

nkubala commented Aug 1, 2019

hey @seblegall, thanks for all the work you put in on this! after some discussion on #2290, I think we've decided to abandon the sha256 approach for tagging, as it doesn't really add much value over the other tagging strategies we have in skaffold. i'm gonna go ahead and close this PR, but wanted to thank you again for your hard work here :) and if you feel up to helping out with the plan we laid out in the issue I linked, don't hesistate!

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.

sha256 tagger doesn't actually compute checksum

5 participants