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.

Sign up for GitHub

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

Jump to bottom

Move all 'common' code to 'pkg' #11331

Merged
merged 1 commit into from May 6, 2020
Merged

Move all 'common' code to 'pkg' #11331

merged 1 commit into from May 6, 2020

Conversation

soumynathan
Copy link
Contributor

This PR moves the common packages to the pkg folder.

Fixes: #11293
Signed-off-by: Swaminathan Vasudevan svasudevan@suse.com

@soumynathan soumynathan requested review from a team May 5, 2020 01:31
@soumynathan soumynathan requested review from a team as code owners May 5, 2020 01:31
@soumynathan soumynathan requested a review from a team May 5, 2020 01:31
@soumynathan soumynathan requested review from a team as code owners May 5, 2020 01:31
@maintainer-s-little-helper
Copy link

Please set the appropriate release note label.

@maintainer-s-little-helper maintainer-s-little-helper bot added this to In progress in 1.8.0 May 5, 2020
@joestringer joestringer added the release-note/misc This PR makes changes that have no direct user impact. label May 5, 2020
tklauser
tklauser requested changes May 5, 2020
View reviewed changes
Copy link
Member

@tklauser tklauser left a comment •
edited

Choose a reason for hiding this comment

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

The maptool build failure on Travis CI looks related to this PR: https://travis-ci.com/github/cilium/cilium/jobs/327849061

@soumynathan soumynathan force-pushed the move-all-common-code-to-pkg branch from b53bbf9 to 4a3df6a Compare May 5, 2020 15:00
@coveralls
Copy link

coveralls commented May 5, 2020 •
edited

Coverage Status

Coverage increased (+0.01%) to 44.442% when pulling 3c784c7 on soumynathan:move-all-common-code-to-pkg into 3ec3d25 on cilium:master.

@tklauser
Copy link
Member

tklauser commented May 5, 2020

test-me-please

@joestringer
Copy link
Member

Precheck failed,

15:13:25  + cd /home/jenkins/workspace/Cilium-PR-Ginkgo-Tests-Validated/src/github.com/cilium/cilium/
15:13:25  + make jenkins-precheck
15:13:25  docker-compose -f test/docker-compose.yml -p Cilium-PR-Ginkgo-Tests-Validated-$BUILD_NUMBER run --rm precheck
15:13:26  Creating network "cilium-pr-ginkgo-tests-validated-19489_default" with the default driver
15:13:29  ineffassign .
15:13:29  contrib/scripts/check-logging-subsys-field.sh
15:13:29  contrib/scripts/check-fmt.sh
15:13:30  Unformatted Go source code:
...

@soumynathan
Move all 'common' code to 'pkg'
3c784c7 
This PR moves the common packages to the pkg folder.

Fixes: cilium#11293
Signed-off-by: Swaminathan Vasudevan <svasudevan@suse.com>
@soumynathan soumynathan force-pushed the move-all-common-code-to-pkg branch from 4a3df6a to 3c784c7 Compare May 6, 2020 06:03
@pchaigno
Copy link
Member

pchaigno commented May 6, 2020

test-me-please

@aanm aanm requested a review from tklauser May 6, 2020 08:15
tklauser
tklauser reviewed May 6, 2020
View reviewed changes
Copy link
Member

@tklauser tklauser left a comment

Choose a reason for hiding this comment

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

Please also drop the ./common/... entry from the govet target in Makefile (can be done as a follow-up PR):

cilium/Makefile

Line 403 in 5aeea78

./common/... \

joestringer
joestringer approved these changes May 6, 2020
View reviewed changes
Copy link
Member

@joestringer joestringer left a comment

Choose a reason for hiding this comment

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

Thanks for picking this up! Looks like a good first cut.

I think a follow-up could revise what's left in pkg/common and determine whether they would be more appropriate in a combined package with other bits, because common is so generic that it can easily magnet anything that doesn't fit somewhere. Also I saw the pkg/common/build.sh which can be removed.

I also wonder about pkg/addressing vs. pkg/types, seems like there's some commonality here which we could review to see if it makes sense to combine these (potentially also related, pkg/cidr.. For the L2 stuff we have pkg/mac.

None of this is blocking, we can further iterate on top of this PR as a base.

@joestringer
Copy link
Member

There's been 3 reviews and the changes are trivial for remaining codeowners; it's also passing CI. Merging.

@joestringer joestringer closed this May 6, 2020
@joestringer joestringer reopened this May 6, 2020
@joestringer joestringer merged commit e374eac into cilium:master May 6, 2020
1.8.0 automation moved this from In progress to Merged May 6, 2020
@soumynathan
Copy link
Contributor Author

Thanks for picking this up! Looks like a good first cut.

I think a follow-up could revise what's left in pkg/common and determine whether they would be more appropriate in a combined package with other bits, because common is so generic that it can easily magnet anything that doesn't fit somewhere. Also I saw the pkg/common/build.sh which can be removed.

I also wonder about pkg/addressing vs. pkg/types, seems like there's some commonality here which we could review to see if it makes sense to combine these (potentially also related, pkg/cidr.. For the L2 stuff we have pkg/mac.

None of this is blocking, we can further iterate on top of this PR as a base.

Makes sense.

@pchaigno pchaigno mentioned this pull request May 7, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tklauser tklauser tklauser approved these changes

@joestringer joestringer joestringer approved these changes

@aanm aanm aanm approved these changes

Assignees
No one assigned
Labels
release-note/misc This PR makes changes that have no direct user impact.
Projects
No open projects
1.8.0
  
Merged
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Move all common/ code into pkg/
6 participants
@soumynathan @coveralls @tklauser @joestringer @pchaigno @aanm