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

fix: explicit fields for CreateSecurityGroupRequest #10

Merged
merged 1 commit into from
May 30, 2024
Merged

Conversation

si-mm
Copy link
Contributor

@si-mm si-mm commented Apr 26, 2024

Change the CreateSecurityGroupRequest to include fields of specific data required for creating a security group. This deviates from the request objects only using a "Template" field. The idea is here is so that manifold has an explicit contract and handles marshaling. This PR's implementation of marshaling could be improved (e.g. helper funcs) but this is more of a POC.

Status quo requires the caller of manifold to marshal a string payload everywhere.

Could this be a pattern we want to follow going forward?

Moving forward with SetTemplate() by the caller. This PR just adds new security group rules.

@si-mm si-mm requested a review from masonkatz April 26, 2024 18:59
@masonkatz
Copy link
Contributor

What is the universe of fields in the template?

@si-mm
Copy link
Contributor Author

si-mm commented Apr 29, 2024

The basic fields I've interacted with are:

name
description
group     

rule {
    protocol
    rule_type
}

Not all are needed or required for creating a security group.

After looking at my changes again, we could keep Template and use it as an escape hatch until most/all fields are supported.

@si-mm
Copy link
Contributor Author

si-mm commented May 8, 2024

I kept the template field just in case. And I can see cases made as to where the ini-like template format should be serialized. In the manifold client or sifi backend.

I believe since the manifold client/api has it's own json interface, it should stick with that and let the backend take said json payload and generate the according template data. Either via the Template field or the various other fields provided.

cloud/service_sg.go Outdated Show resolved Hide resolved
@si-mm si-mm force-pushed the sg_changes branch 2 times, most recently from f2a4290 to fba736e Compare May 28, 2024 13:51
cloud/sg.go Show resolved Hide resolved
@masonkatz masonkatz merged commit 19a5612 into main May 30, 2024
0 of 2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants