-
Notifications
You must be signed in to change notification settings - Fork 25
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
Creating and Getting GCP Integration #18
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nicely done, I like that you opened a PR to start the code review and you also added
an explanation, thank you @mjunglw - I have a few things we have to change.
1192cc2
to
349099f
Compare
349099f
to
052a771
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
api/integrations.go
Outdated
// azureCFG | ||
|
||
// azureAL - Azure Activity Log integration type | ||
// azureAL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now that I think about it, you have to be very careful here, the fact that you add a single
constant with the keyword iota
it means that you will have that constant set to 0
(gcpCFG = 0
)
I bet you that this is a bug! if we call gcpCFG.String()
it will return AWS_CFG
That is why I always recommend using this pattern:
var integrationTypes = map[integrationType]string{}
Mainly because you won't have to worry about the order of the array or index etc.
You just define them and it automatically works.
I would suggest implementing this pattern instead 👆🏽
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Look at this shippable job link
=== RUN TestCreateGCPConfigIntegration
TestCreateGCPConfigIntegration: integrations_test.go:78:
Error Trace: integrations_test.go:78
Error: Not equal:
expected: "GCP_CFG"
actual : "AWS_CFG"
Diff:
--- Expected
+++ Actual
@@ -1 +1 @@
-GCP_CFG
+AWS_CFG
Test: TestCreateGCPConfigIntegration
Messages: a new GCP integration should match its type
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, you were right. Now I see. For refactoring / adding new features, the map seems simpler.
const ( | ||
// GcpProject level integration with GCP | ||
GcpProject gcpResourceLevel = iota | ||
|
||
// GcpOrganization level integration with GCP | ||
GcpOrganization | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since all these are global constants, we should make sure to call them appropriately,
It is a good practice to be over descriptive with these kinds of things, just like we
sometimes are with function names. Especially since these ones are public 😉
I would propose:
const ( | |
// GcpProject level integration with GCP | |
GcpProject gcpResourceLevel = iota | |
// GcpOrganization level integration with GCP | |
GcpOrganization | |
) | |
const ( | |
// a project level integration with GCP | |
GcpProjectLevelIntegration gcpResourceLevel = iota | |
// an organization level integration with GCP | |
GcpOrganizationLevelIntegration | |
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let us do this in a further PR, I want us to keep iteration over this. Great work James!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed a bit with @afiunelw and @lwmobeent but we were cut a little short.
Thought it would be best to raise a PR so everyone could see.
CreateGCPConfigIntegration
calls
/api/v2/external/integrations
Lacework API to create a GCP integrationExample (similarly in integration_test.go):
GetGCPConfigIntegration
calls
/api/v2/external/integrations/<INTG_GUID>
Lacework API to get a GCP integration with integration guidExample (similarly in integration_test.go):
Go's Constants / Enums
Using
iota
in go:Discussed with @afiunelw and @lwmobeent. Referenced here on golang github and here on a random website that I googled.
For Integration Type:
For GCP resource level integration (customers will tell us whether they want a project or organization level integration):
Closes #17