Skip to content
This repository was archived by the owner on Jun 29, 2022. It is now read-only.

pkg/components: add Inspektor Gadget component#1076

Merged
iaguis merged 5 commits intomasterfrom
iaguis/inspektor-gadget
Oct 22, 2020
Merged

pkg/components: add Inspektor Gadget component#1076
iaguis merged 5 commits intomasterfrom
iaguis/inspektor-gadget

Conversation

@iaguis
Copy link
Contributor

@iaguis iaguis commented Oct 14, 2020

Fixes #1063

@iaguis iaguis force-pushed the iaguis/inspektor-gadget branch 4 times, most recently from 9dcad0b to ff3fc5b Compare October 14, 2020 12:58
invidian
invidian previously approved these changes Oct 15, 2020
View reviewed changes
Copy link
Member

@invidian invidian left a comment

Choose a reason for hiding this comment

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

Just some nits, otherwise LGTM, but I'm curious why CI is failing.

@iaguis iaguis force-pushed the iaguis/inspektor-gadget branch 4 times, most recently from 94eef77 to 150bcc1 Compare October 15, 2020 15:14
@iaguis
Copy link
Contributor Author

iaguis commented Oct 16, 2020

I've changed the namespace to gadget, for some reasoning see inspektor-gadget/inspektor-gadget#140 (comment).

I'm curious why CI is failing.

I was using WaitForDeployment() instead of ẀaitForDaemonSet() 🤦

@iaguis iaguis force-pushed the iaguis/inspektor-gadget branch from 150bcc1 to 6dac435 Compare October 16, 2020 10:44
@iaguis
Copy link
Contributor Author

iaguis commented Oct 16, 2020

I've changed the namespace to gadget, for some reasoning see kinvolk/inspektor-gadget#140 (comment).

After thinking about it some more and discussing with others I think what makes most sense is to keep IG in the kube-system namespace so it works out of the box with the current kubectl gadget plugin. Since it will have its own PSP this should be OK. If/when upstream IG changes its namespace to gadget we can change it too.

I changed the PR accordingly.

@invidian
Copy link
Member

invidian commented Oct 16, 2020

Odd that CI didn't get triggered.

invidian
invidian previously requested changes Oct 19, 2020
View reviewed changes
Copy link
Member

@invidian invidian left a comment

Choose a reason for hiding this comment

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

Just one broken thing, otherwise LGTM.

@iaguis iaguis force-pushed the iaguis/inspektor-gadget branch from 6dac435 to 8129b0b Compare October 19, 2020 16:44
knrt10
knrt10 reviewed Oct 22, 2020
View reviewed changes
knrt10
knrt10 reviewed Oct 22, 2020
View reviewed changes
knrt10
knrt10 suggested changes Oct 22, 2020
View reviewed changes
Copy link
Contributor

@knrt10 knrt10 left a comment

Choose a reason for hiding this comment

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

One small NIT, rest LGTM

@iaguis iaguis force-pushed the iaguis/inspektor-gadget branch from 8129b0b to 10705c9 Compare October 22, 2020 11:09
@iaguis iaguis requested review from invidian and knrt10 October 22, 2020 11:12
knrt10
knrt10 approved these changes Oct 22, 2020
View reviewed changes
Copy link
Contributor

@knrt10 knrt10 left a comment

Choose a reason for hiding this comment

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

Looks great. +1

@knrt10 knrt10 dismissed invidian’s stale review October 22, 2020 11:54

change addressed

@iaguis
Copy link
Contributor Author

iaguis commented Oct 22, 2020

I'll take @invidian's lgtm and @knrt10's one and merge this.

@iaguis iaguis merged commit 3c901c0 into master Oct 22, 2020
@iaguis iaguis deleted the iaguis/inspektor-gadget branch October 22, 2020 13:35
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@invidian invidian Awaiting requested review from invidian

1 more reviewer

@knrt10 knrt10 knrt10 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Add Inspektor Gadget component

3 participants

@iaguis @invidian @knrt10