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

Combination of three paprs #9

Closed
wants to merge 2 commits into from
Closed

Conversation

cevich
Copy link
Member

@cevich cevich commented Nov 2, 2017

Very rough ATM, minimal cleanup done, but maybe could work?

@rh-atomic-bot
Copy link
Collaborator

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

  • Value 'golang' is not unique. Previous path: '/packages/3'. Path: '/packages/19'.
  • Value 'device-mapper-devel' is not unique. Previous path: '/packages/7'. Path: '/packages/14'.
  • Value 'gpgme-devel' is not unique. Previous path: '/packages/6'. Path: '/packages/20'.
  • Value 'git' is not unique. Previous path: '/packages/0'. Path: '/packages/16'.
  • Value 'glib2-devel' is not unique. Previous path: '/packages/10'. Path: '/packages/17'.
  • Value 'libassuan-devel' is not unique. Previous path: '/packages/8'. Path: '/packages/21'.
  • Value 'libseccomp-devel' is not unique. Previous path: '/packages/5'. Path: '/packages/22'..

@rh-atomic-bot
Copy link
Collaborator

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

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

@cevich
Copy link
Member Author

cevich commented Nov 2, 2017

ohh, forgot to add more memory.

.papr.yml Outdated
- master

host:
distro: fedora/26/cloud
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not fedora/26/atomic here? Isn't that more aligned with what this tool is targeted at?

For an example of how this can work, see the pattern in projectatomic/atomic, where we just build and e.g. pylint in a container, but then install onto the host and run the integration tests natively. (And just like projectatomic/atomic, a second fedora/26/cloud testsuite can always be added).

Copy link
Member Author

Choose a reason for hiding this comment

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

@jlebon because IRC 😀

@rh-atomic-bot
Copy link
Collaborator

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

  • Key 'spec' was not defined. Path: '/host'..

.papr.yml Outdated
@@ -1,29 +1,34 @@
branches:
- master
- auto
- try
Copy link
Contributor

Choose a reason for hiding this comment

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

Drop this hunk?

@cevich cevich force-pushed the pr_no_7 branch 2 times, most recently from 06f960c to d64221f Compare November 2, 2017 15:07
@cevich cevich changed the title Merge PR 6 and PR 7 Combination of three paprs Nov 2, 2017
@jlebon
Copy link
Contributor

jlebon commented Nov 2, 2017

bot, retest this please

2 similar comments
@jlebon
Copy link
Contributor

jlebon commented Nov 2, 2017

bot, retest this please

@jlebon
Copy link
Contributor

jlebon commented Nov 2, 2017

bot, retest this please

@jlebon
Copy link
Contributor

jlebon commented Nov 2, 2017

bot, retest this please

Makefile Outdated

testunit:
$(GO) test -tags "$(BUILDTAGS)" -cover $(PACKAGES)

localintegration: clean binaries test-binaries
localintegration: binaries test-binaries
./test/test_runner.sh ${TESTFLAGS}
Copy link
Member Author

Choose a reason for hiding this comment

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

@mheon I remove clean from this. That allowed tests to run (b/c clean removes bin/common). But maybe that was bad to do.

Copy link
Member

Choose a reason for hiding this comment

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

Binaries should be rebuilding conmon. It didn't look like it was. I'll look into it.

Copy link
Member

Choose a reason for hiding this comment

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

I am removing both the clean and the binaries. Currently we don't build anywhere but make all or make binaries. lets keep it that way.

Copy link
Member Author

Choose a reason for hiding this comment

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

kk, so make -C $GOSRC binaries install.tools all TAGS=${TAGS} and I commented out the original one.

@cevich
Copy link
Member Author

cevich commented Nov 2, 2017

@rhatdan @mheon and @baude helped me get this one building locally. It's running well all the way through executing the integration tests. So the papr stuff is probably working fine (when papr starts working again we'll know for sure) and it's code/makefile stuff that's breaking now. Heading to get lunch to give jlebon some time.

@cevich
Copy link
Member Author

cevich commented Nov 2, 2017

ahh there she goes! thx @jlebon !

@cevich cevich force-pushed the pr_no_7 branch 3 times, most recently from 1e2abb9 to e52ed1e Compare November 2, 2017 18:55
@rh-atomic-bot
Copy link
Collaborator

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

  • Value 'libseccomp-devel' is not unique. Previous path: '/packages/5'. Path: '/packages/22'.
  • Value 'glib2-devel' is not unique. Previous path: '/packages/10'. Path: '/packages/17'.
  • Value 'golang' is not unique. Previous path: '/packages/3'. Path: '/packages/19'.
  • Value 'device-mapper-devel' is not unique. Previous path: '/packages/7'. Path: '/packages/14'.
  • Value 'gpgme-devel' is not unique. Previous path: '/packages/6'. Path: '/packages/20'.
  • Value 'libassuan-devel' is not unique. Previous path: '/packages/8'. Path: '/packages/21'.
  • Value 'git' is not unique. Previous path: '/packages/0'. Path: '/packages/16'..

.papr.yml Outdated
- gpgme-devel
- libassuan-devel
- libseccomp-devel
- libselinux-devel
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like papr doens't like the duplicates.

Copy link
Member Author

Choose a reason for hiding this comment

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

no, makes it very angry 😀 s'okay, my own damn fault (again).

@cevich
Copy link
Member Author

cevich commented Nov 2, 2017

OMG! There are no quotes around ${TAGS} so it's treating multiple SPACE-SEPARATED tags as multiple arguments, not multiple tags...adding J$%*%&&388#! quotes...

@cevich
Copy link
Member Author

cevich commented Nov 2, 2017

bot, retest this please

@cevich
Copy link
Member Author

cevich commented Nov 2, 2017

bot, retest this please

@rh-atomic-bot
Copy link
Collaborator

☔ The latest upstream changes (presumably #6) made this pull request unmergeable. Please resolve the merge conflicts.

@rhatdan
Copy link
Member

rhatdan commented Nov 2, 2017

Needs a rebase.

@cevich
Copy link
Member Author

cevich commented Nov 3, 2017

@rhatdan @mheon said there's no reason other than "sexiness" to do the build inside a container. I'm not opposed to it in practice...except for...your way introduces a tight-coupling between the host-setup, and the testing. IMHO, best to have a gap there, b/c it makes it easier to swap one host environment for another. And/Or, having multiple VMs running the same testing steps (supported by papr). I think @baude had in mind to do something like this

All that said, are you married to the container-way? (I'm happy to amend this PR to only realize the division of labor part.)

@cgwalters
Copy link
Contributor

It's not sexiness - you should always do builds in containers, and don't have that stuff on your host. Even for workstations. There are many reasons (security), and you'll be a lot less likely to hit package management disasters. And further once you do this, it's just as easy to spin up e.g. CentOS/RHEL/whatever containers and use those.

@cevich
Copy link
Member Author

cevich commented Nov 3, 2017

@cgwalters or...we do it both ways :D

@cgwalters
Copy link
Contributor

FWIW I think "Fedora Atomic" is Fedora too. So I'd title it "Test in Fedora Cloud and Fedora Atomic".

@cevich
Copy link
Member Author

cevich commented Nov 3, 2017

FWIW I think "Fedora Atomic" is Fedora too. So I'd title it "Test in Fedora Cloud and Fedora Atomic".

True true, an easy fix, thx.

@rhatdan
Copy link
Member

rhatdan commented Nov 3, 2017

I would prefer if the tests would happen outside of the container, since things like SELinux will not be tested if running inside of a container.

@cevich
Copy link
Member Author

cevich commented Nov 3, 2017

@rhatdan latest push does both, in and out of a container. However @baude wants to take this over, so closing...

@cevich cevich closed this Nov 3, 2017
@cgwalters
Copy link
Contributor

@cgwalters
Copy link
Contributor

Note specifically the use of e.g. ostree admin unlock etc.

@jlebon
Copy link
Contributor

jlebon commented Nov 3, 2017

Another great example of this pattern that I mentioned earlier is projectatomic/atomic, which @baude is intimately familiar with :). Specifically, notice how the PAPR YAML simply calls out the same script for all three platforms.

@cevich
Copy link
Member Author

cevich commented Nov 3, 2017

@jlebon @cgwalters I think we're all in general agreement. Thanks for the links. @baude is taking this over.

@cevich cevich deleted the pr_no_7 branch September 17, 2018 21:26
baude referenced this pull request in baude/podman Aug 31, 2019
Move libraries from cni only used by plugins.
jwhonce referenced this pull request in jwhonce/podman Nov 5, 2019
Add a bunch of endpoints
@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 27, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 27, 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.

6 participants