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

Allow contributors to launch integration tests against local registry #1014

Conversation

antechrestos
Copy link
Contributor

@antechrestos antechrestos commented Jan 30, 2020

Fixes #1012

Description

This change allows contributors to launch integration tests with a local registry.

The idea is to check whether IMAGE_REPO parameter and

  • if it starts with gcr.io, keep the previous thing
  • if it starts with anything else, the program will crawl through dockerfiles, save them as temporary files, import the based gcr.io public images and push them to the target repo then launch the test. The rollback of savec files is made as a defer function

Please also note that I isolated the code of test launching in a dedicated function as os.Exit and defer cannot be mixed (as os.Exit exists without calling defered functions).

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

  • Includes [unit tests] Not needed
  • Adds integration tests Not needed

See the contribution guide for more details.

Reviewer Notes

  • The code flow looks good.
  • Unit tests and or integration tests added.

@googlebot googlebot added the cla: yes CLA signed by all commit authors label Jan 30, 2020
@antechrestos antechrestos force-pushed the feature/add_documentation_for_local_integration_test branch 2 times, most recently from ea48aaf to a20779e Compare January 30, 2020 23:16
@antechrestos
Copy link
Contributor Author

@tejal29 can I have the kokoro result?
Isn't any way to make him post the result in the discussions? Since it is able to remove a label, it should be able to post a message with an attachment I guess

@samos123 samos123 self-requested a review February 2, 2020 03:25
@samos123
Copy link
Contributor

samos123 commented Feb 2, 2020

Nice pull request. Significant improvement over how it was before. We're planning to get rid of Kokoro since Travis CI seems to be doing the job well enough and has public build logs.

I don't know why Kokoro is failing. @tejal29 can you take a look at why Kokoro is failing? this is the short error log:

    --- FAIL: TestRun/test_Dockerfile_test_hardlink (53.80s)
        cmd.go:41: [container-diff diff --no-cache daemon://gcr.io/kaniko-test/docker-dockerfile_test_hardlink gcr.io/kaniko-test/kaniko-dockerfile_test_hardlink -q --type=file --type=metadata --json]
        cmd.go:42: fatal error: concurrent map iteration and map write

integration/config.go Outdated Show resolved Hide resolved
@antechrestos antechrestos force-pushed the feature/add_documentation_for_local_integration_test branch from a20779e to d843c7d Compare February 2, 2020 15:11
@antechrestos
Copy link
Contributor Author

antechrestos commented Feb 2, 2020

@samos123 The issue you point must be the one @cvgw posted

@antechrestos
Copy link
Contributor Author

@samos123 I've submitted a fix

@samos123
Copy link
Contributor

samos123 commented Feb 3, 2020

Ah so we should wait for a new container-diff binary that includes that fix right? Either way, I've triggered kokoro just now to run again. I think it's safe to ignore kokoro in this case but let's wait for @cvgw or @tejal29 feedback.

@antechrestos
Copy link
Contributor Author

antechrestos commented Feb 3, 2020

@samos123 Luckily (for me) kokoro run successfully 😄

Copy link
Member

@tejal29 tejal29 left a comment

Choose a reason for hiding this comment

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

with nits.

DEVELOPMENT.md Outdated Show resolved Hide resolved
integration/integration_test.go Outdated Show resolved Hide resolved
integration/integration_test.go Outdated Show resolved Hide resolved
integration/integration_test.go Outdated Show resolved Hide resolved
integration/migrate_gcs.go Outdated Show resolved Hide resolved
@antechrestos antechrestos force-pushed the feature/add_documentation_for_local_integration_test branch 4 times, most recently from 57ff33b to e281321 Compare February 5, 2020 13:03
@antechrestos
Copy link
Contributor Author

@tejal29 done.

Other point, the integration test TestRun/test_Dockerfile_test_copy_symlink has random behaviour. I don't know why but it passed the first time, then I had to repush cause my code was not well formatted (one excessive space), then it failed twice and succeeded the third time.

The two failure were due to

--- FAIL: TestRun/test_Dockerfile_test_copy_symlink (5.85s)

        integration_test.go:208: diff = [

              {

                "Image1": "localhost:5000/docker-dockerfile_test_copy_symlink",

                "Image2": "localhost:5000/kaniko-dockerfile_test_copy_symlink",

                "DiffType": "File",

                "Diff": {

                  "Adds": [

                    {

                      "Name": "/tmp/link",

                      "Size": -1

                    }

                  ],

                  "Dels": null,

                  "Mods": [

                    {

                      "Name": "/tmp",

                      "Size1": 6,

                      "Size2": 11

                    }

                  ]

                }

              },

              {

                "Image1": "localhost:5000/docker-dockerfile_test_copy_symlink",

                "Image2": "localhost:5000/kaniko-dockerfile_test_copy_symlink",

                "DiffType": "Metadata",

                "Diff": {

                  "Adds": [],

                  "Dels": []

                }

              }

            ]

        integration_test.go:211: []integration.diffOutput differ (-got, +want):   []integration.diffOutput{

              	{

              		Image1:   "localhost:5000/docker-dockerfile_test_copy_symlink",

              		Image2:   "localhost:5000/kaniko-dockerfile_test_copy_symlink",

              		DiffType: "File",

              		Diff: &integration.fileDiffResult{

            - 			Adds: []integration.fileDiff{{Name: "/tmp/link", Size: -1}},

            + 			Adds: nil,

              			Dels: nil,

              		},

              	},

              	{Image1: "localhost:5000/docker-dockerfile_test_copy_symlink", Image2: "localhost:5000/kaniko-dockerfile_test_copy_symlink", DiffType: "Metadata", Diff: &integration.metaDiffResult{Adds: []string{}, Dels: []string{}}},

              }

This change allows user to launch integration tests with a local registry

Fixes GoogleContainerTools#1012
@antechrestos antechrestos force-pushed the feature/add_documentation_for_local_integration_test branch from e281321 to 3e2221c Compare February 6, 2020 12:36
@antechrestos
Copy link
Contributor Author

antechrestos commented Feb 6, 2020

@tejal29 looks like you fixed the issue. I've rebased my work and it passed directly. I hope it was not due to luck 😏 .

@tejal29
Copy link
Member

tejal29 commented Feb 6, 2020

Thank you much!

@tejal29 tejal29 merged commit 343f418 into GoogleContainerTools:master Feb 6, 2020
@antechrestos antechrestos deleted the feature/add_documentation_for_local_integration_test branch February 7, 2020 10:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes CLA signed by all commit authors kokoro:run
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add documentation about local integration tests
5 participants