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

feat: add SMI log collector #71

Merged
merged 17 commits into from
Jul 17, 2021
Merged

feat: add SMI log collector #71

merged 17 commits into from
Jul 17, 2021

Conversation

johnsonshi
Copy link
Contributor

Add log collector for Service Mesh Interface
logs, CustomResourceDefinitions, and custom
resources.

Related to PR #48.

Resolves issue #67.

Signed-off-by: Johnson Shi [email protected]

@johnsonshi
Copy link
Contributor Author

Hey @Tatsinnit, I created a draft PR outlining the types of logs of SMI CRDs that I'll be collecting. Let me know what you think plus your thoughts on how the logs are organized.

@johnsonshi johnsonshi marked this pull request as ready for review June 22, 2021 21:11
@johnsonshi
Copy link
Contributor Author

Ready now

@Tatsinnit Tatsinnit linked an issue Jun 24, 2021 that may be closed by this pull request
feat: add SMI collector in Periscope #67
Closed
Tatsinnit
Tatsinnit requested changes Jun 24, 2021
View reviewed changes
Copy link
Member

@Tatsinnit Tatsinnit left a comment

Choose a reason for hiding this comment

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

💡 Thank you so much for this, I have added initial comments below. Really appreciate it.

deployment/aks-periscope.yaml Outdated Show resolved Hide resolved
pkg/collector/smi_collector.go Outdated Show resolved Hide resolved
deployment/aks-periscope.yaml Outdated Show resolved Hide resolved
pkg/collector/smi_collector.go Outdated Show resolved Hide resolved
pkg/collector/smi_collector.go Outdated Show resolved Hide resolved
pkg/collector/smi_collector.go Outdated Show resolved Hide resolved
pkg/collector/smi_collector.go Outdated Show resolved Hide resolved
pkg/collector/smi_collector.go Outdated Show resolved Hide resolved
pkg/collector/smi_collector.go Outdated Show resolved Hide resolved
pkg/collector/smi_collector.go Outdated Show resolved Hide resolved
arnaud-tincelin
arnaud-tincelin suggested changes Jul 7, 2021
View reviewed changes
pkg/collector/smi_collector.go Outdated Show resolved Hide resolved
pkg/collector/smi_collector.go Outdated Show resolved Hide resolved
pkg/collector/smi_collector.go Outdated Show resolved Hide resolved
davidkydd and others added 10 commits July 7, 2021 09:17
@davidkydd @johnsonshi
Add text=auto in .gitattributes (Azure#70)
fc965f2 
add default .gitattributes file and git add --renormalize
@johnsonshi
feat: add SMI log collector
45af0a3 
Add log collector for Service Mesh Interface
logs, CustomResourceDefinitions, and custom
resources.

Related to PR Azure#48.

Resolves issue Azure#67.

Signed-off-by: Johnson Shi <[email protected]>
@shalier @johnsonshi
Add logic to make flags mutually exclusive
1366024
@johnsonshi
Reverse deployment/aks-periscope.yaml changes.
5053bcb 
Signed-off-by: Johnson Shi <[email protected]>
@johnsonshi
Rename func to collectSmiCrds
5590524 
Signed-off-by: Johnson Shi <[email protected]>
@johnsonshi
Clarify log messages and error returns
b06f187 
Signed-off-by: Johnson Shi <[email protected]>
@johnsonshi
Rename func collectSmiCustomResourcesFromNamespace
a9a8ebe 
Signed-off-by: Johnson Shi <[email protected]>
@johnsonshi
Add comment about directories with namespace names
28466ef 
Signed-off-by: Johnson Shi <[email protected]>
@johnsonshi
Add test for FilterSliceElemsWithTestPredicate
848229a 
Signed-off-by: Johnson Shi <[email protected]>
@johnsonshi
Merge smi collector
e92aa86 
Signed-off-by: Johnson Shi <[email protected]>
@johnsonshi
Merge Dockerfile
8b0e991 
Signed-off-by: Johnson Shi <[email protected]>
@johnsonshi
Copy link
Contributor Author

Hi @Tatsinnit @arnaud-tincelin, I've resolved all conflicts and addressed @arnaud-tincelin's additional comments. I think this can now be merged - thank you!!! 😀🙏🙏

Tatsinnit
Tatsinnit requested changes Jul 8, 2021
View reviewed changes
Copy link
Member

@Tatsinnit Tatsinnit left a comment •
edited
Loading

Choose a reason for hiding this comment

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

👍 Rest looks good @johnsonshi , Please note: once you made leftover recommended changes and especially not checking in with credentials :)

cc: @arnaud-tincelin for rest. Thank you so much, 🙏

deployment/aks-periscope.yaml Outdated Show resolved Hide resolved
deployment/aks-periscope.yaml Outdated Show resolved Hide resolved
tests/aksperiscopeintegration_test.go Outdated Show resolved Hide resolved
Tatsinnit
Tatsinnit reviewed Jul 8, 2021
View reviewed changes
pkg/utils/helper.go Outdated Show resolved Hide resolved
@johnsonshi
Address errors comments
376c327 
Signed-off-by: Johnson Shi <[email protected]>
Tatsinnit
Tatsinnit approved these changes Jul 14, 2021
View reviewed changes
Copy link
Member

@Tatsinnit Tatsinnit left a comment •
edited
Loading

Choose a reason for hiding this comment

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

👍 LGTM, thank you so much for this.

I will defer to other maintainer folks for one last eyeballing, let’s merge it after that. Cc: @arnaud-tincelin since you’ve reviewed this before. + Cc: @JunSun17 , @davidkydd

Thanks 🙏

arnaud-tincelin
arnaud-tincelin suggested changes Jul 16, 2021
View reviewed changes
deployment/aks-periscope.yaml Outdated Show resolved Hide resolved
@johnsonshi
Filter smi crds directly
3b6b973 
Signed-off-by: Johnson Shi <[email protected]>
@Tatsinnit Tatsinnit merged commit a1bdc52 into Azure:master Jul 17, 2021
@johnsonshi johnsonshi deleted the smi branch July 19, 2021 16:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SanyaKochhar SanyaKochhar SanyaKochhar left review comments

@arnaud-tincelin arnaud-tincelin arnaud-tincelin approved these changes

@Tatsinnit Tatsinnit Tatsinnit approved these changes

@davidkydd davidkydd Awaiting requested review from davidkydd

@JunSun17 JunSun17 Awaiting requested review from JunSun17

Assignees

@johnsonshi johnsonshi

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

feat: add SMI collector in Periscope
6 participants
@johnsonshi @Tatsinnit @arnaud-tincelin @SanyaKochhar @davidkydd @shalier