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

Initial gingko implementation #261

Closed
wants to merge 1 commit into from
Closed

Conversation

baude
Copy link
Member

@baude baude commented Jan 24, 2018

Signed-off-by: baude [email protected]

@baude baude changed the title Initial gingko work [do not merge] Initial gingko work [do not merge or review] Jan 24, 2018
@baude baude force-pushed the ginkgo branch 5 times, most recently from 1424d2c to 551587f Compare January 25, 2018 00:54
@rh-atomic-bot
Copy link
Collaborator

💥 Invalid .papr.yml: failed to parse 1st testsuite: Schema validation failed:

  • Key 'ostree' was not defined. Path: ''..

@rh-atomic-bot
Copy link
Collaborator

💥 Invalid .papr.yml: failed to parse 1st testsuite: Schema validation failed:

  • Key 'ostree' was not defined. Path: ''..

@rh-atomic-bot
Copy link
Collaborator

💥 Invalid .papr.yml: failed to parse 1st testsuite: Schema validation failed:

  • Key 'ostree' was not defined. Path: ''..

@baude baude force-pushed the ginkgo branch 7 times, most recently from 67fcfaf to ba6d7cc Compare January 25, 2018 19:32
}()
options := &copy.Options{}

importRef, err := alltransports.ParseImageName(fmt.Sprintf("docker://%s", imageName))
Copy link
Member

Choose a reason for hiding this comment

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

Combined with line 167, I think this ends up prepending "docker://" to image twice?

Copy link
Member Author

Choose a reason for hiding this comment

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

thanks

}

exportTo := filepath.Join(p.ArtifactPath, "image")
exportRef, err := alltransports.ParseImageName(exportTo)
Copy link
Member

Choose a reason for hiding this comment

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

You'll need to prepend "dir:" to exportTo here.

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed

return errors.Errorf("error parsing image name: %v", err)
}

importFrom := fmt.Sprintf("dir://%s", filepath.Join(p.ArtifactPath), "image")
Copy link
Member

Choose a reason for hiding this comment

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

This should be "dir:", not "dir://". Only the "docker" transport expects a "//".

Copy link
Member

Choose a reason for hiding this comment

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

The argument list for that filepath.Join() call doesn't include image the way the one on line 189 does?

os.Exit(1)
}

names := append(destImage.Names, image, image)
Copy link
Member

Choose a reason for hiding this comment

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

No point in appending image twice here, since SetNames deduplicates them anyway.

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed

err = copy.Image(policyContext, ref, importRef, options)
if err != nil {
logrus.Errorf("error importing %s: %v", importFrom, err)
os.Exit(1)
Copy link
Member

Choose a reason for hiding this comment

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

This bypasses any deferred functions, in case doing so could create problems elsewhere.

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah caught that late

destImage, err1 := storage.Transport.GetStoreImage(store, ref)
if err1 != nil {
logrus.Errorf("error finding image: %v", err1)
os.Exit(1)
Copy link
Member

Choose a reason for hiding this comment

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

This bypasses any deferred functions, in case doing so could create problems elsewhere.

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed

@baude baude force-pushed the ginkgo branch 3 times, most recently from 1d8bf34 to 24bd688 Compare January 26, 2018 20:12
@baude baude changed the title Initial gingko work [do not merge or review] Initial gingko implementation Jan 26, 2018
@baude
Copy link
Member Author

baude commented Jan 26, 2018

@rhatdan , @mheon , @umohnani8, @TomSweeneyRedHat PTAL

Makefile Outdated
localintegration: test-binaries
bash -i ./test/test_runner.sh ${TESTFLAGS}
ginkgo -v test/e2e/.
Copy link
Member

Choose a reason for hiding this comment

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

Don't we need test_runner.sh at least for a little while longer?

Copy link
Member Author

Choose a reason for hiding this comment

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

as i said in the commit message, i disabled the bats tests in ubuntu as we are running them in fedora and centos.

Copy link
Member

Choose a reason for hiding this comment

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

I don't think we should do that. I want the full tests to run on all platforms so we catch distro problems.

Copy link
Member

Choose a reason for hiding this comment

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

I'm OK with it if the intent is the eventual removal of BATS tests as they are replaced by Ginkgo tests.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, that is the point ... I removed it in part because the travis tests are so darned long and i wanted to take some burden off the ci. i can re-enable them but given all the ci issues we have had of late, i think it will will contributory to frustration.

Copy link
Member Author

Choose a reason for hiding this comment

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

added back in per request ...

@baude baude force-pushed the ginkgo branch 19 times, most recently from 4b3efbf to b4167a6 Compare January 29, 2018 17:59
@baude
Copy link
Member Author

baude commented Jan 29, 2018

bot, retest this please

This implements the ginkgo integration test framework for
podman.  As tests are migrated from bats to ginkgo, we will
still run both integration suites.  When a test is migrated,
we remove the tests from bats at that time.  All new tests
should be just for the ginkgo framework.

One exception is that we only run the ginkgo suit in the
travis/ubuntu environment.  The CentOS and Fedora PAPR nodes
will more than cover those.

Signed-off-by: baude <[email protected]>
@baude
Copy link
Member Author

baude commented Jan 29, 2018

I'm going to merge this to get the show on the road.

@baude
Copy link
Member Author

baude commented Jan 29, 2018

@rh-atomic-bot r+

@rh-atomic-bot
Copy link
Collaborator

📌 Commit f29f526 has been approved by baude

@rh-atomic-bot
Copy link
Collaborator

⚡ Test exempted: merge already tested.

@baude baude deleted the ginkgo branch December 22, 2019 18:51
@github-actions github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 26, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 26, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants