-
Notifications
You must be signed in to change notification settings - Fork 101
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
Add DeploymentTemplate controller for GitOps support #8001
base: main
Are you sure you want to change the base?
Add DeploymentTemplate controller for GitOps support #8001
Conversation
} | ||
|
||
type OSFileSystem struct { | ||
FileSystem afero.Fs |
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.
instead of writing a filesystem abstraction I added this afero
library. Seems to be widely used, I replaced the OSFileSystem
with it. LMK if I should revert
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.
are there added functionalities that we get using afero
or is the main benefit that's it's an abstraction layer over os
?
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.
instead of writing a filesystem abstraction I added this
afero
library. Seems to be widely used, I replaced theOSFileSystem
with it. LMK if I should revert
The afero repo appears to have no commits for a year even though there are open PRs for it, including one for a CVE. I don't see depndabot PRs being active in the list at all, which suggests that this dependency could open us up to some security issues. None of the issues created in the last year have any comments or responses. If there's a compelling reason for using this library, then let's discuss how to manage it.
On the other hand, Radius already depends on viper, which gives us an indirect dependency on afero.
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.
Good find! I added it here because the stdlib fs
does not include a filesystem abstraction with CreateFile
. The benefit of adding afero
was to not have to implement and maintain this ourselves. Since we already indirectly depend on this library, and that it's a pretty widespread dependency, I'd say we should be okay to use it. Thoughts? @brooke-hamilton
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.
@rynowak thoughts on this?
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.
What's the savings of using this library? I don't have fundamental concerns about it, I just can't tell from the diff what's going to change.
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.
I'd also really like to remove viper as a dependency. We've got a lot of workarounds in place for for behaviors that aren't helpful to us.
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.
I removed afero
as a dependency and wrote a simple filesystem abstraction that we can use
@@ -527,7 +527,6 @@ func Test_Run(t *testing.T) { | |||
}) | |||
|
|||
t.Run("Deployment with missing parameters", func(t *testing.T) { | |||
//t.Skip() |
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.
removing commented out t.Skip()
Radius functional test overview
Click here to see the list of tools in the current test run
Test Status⌛ Building Radius and pushing container images for functional tests... |
pkg/cli/cmd/bicep/generatekubernetesmanifest/generatekubernetesmanifest.go
Outdated
Show resolved
Hide resolved
pkg/cli/cmd/bicep/generatekubernetesmanifest/generatekubernetesmanifest.go
Outdated
Show resolved
Hide resolved
pkg/cli/cmd/bicep/generatekubernetesmanifest/generatekubernetesmanifest.go
Show resolved
Hide resolved
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.
I ran make generate build test
and got 8 failures.
=== FAIL: pkg/cli/cmd/bicep/generatekubernetesmanifest Test_Run/Create_basic_DeploymentTemplate (0.00s)
=== FAIL: pkg/cli/cmd/bicep/generatekubernetesmanifest Test_Run/Create_DeploymentTemplate_with_template_content (0.00s)
=== FAIL: pkg/cli/cmd/bicep/generatekubernetesmanifest Test_Run/Create_DeploymentTemplate_with_Azure_scope (0.00s)
=== FAIL: pkg/cli/cmd/bicep/generatekubernetesmanifest Test_Run/Create_DeploymentTemplate_with_AWS_scope (0.00s)
=== FAIL: pkg/cli/cmd/bicep/generatekubernetesmanifest Test_Run (0.00s)
=== FAIL: pkg/controller/reconciler Test_DeploymentResourceReconciler_Basic (10.01s)
=== FAIL: pkg/controller/reconciler Test_DeploymentTemplateReconciler_Basic (0.01s)
=== FAIL: pkg/controller/reconciler Test_DeploymentTemplateReconciler_FailureRecovery (0.00s)
Here's one example:
=== FAIL: pkg/cli/cmd/bicep/generatekubernetesmanifest Test_Run/Create_DeploymentTemplate_with_Azure_scope (0.00s)
Creating DeploymentTemplate YAML file
generatekubernetesmanifest_test.go:396:
Error Trace: /workspaces/radius/pkg/cli/cmd/bicep/generatekubernetesmanifest/generatekubernetesmanifest_test.go:396
Error: Not equal:
expected: "apiVersion: radapp.io/v1alpha3\nkind: DeploymentTemplate\nmetadata:\n name: azure.yaml\n namespace: radius-system\nspec:\n parameters: '{}'\n providerConfig: '{\"radius\":{\"type\":\"radius\",\"value\":{\"scope\":\"/planes/radius/local/resourceGroups/test-resource-group\"}},\"az\":{\"type\":\"azure\",\"value\":{\"scope\":\"/subscriptions/test-subId/resourceGroups/test-rg\"}},\"deployments\":{\"type\":\"Microsoft.Resources\",\"value\":{\"scope\":\"/planes/radius/local/resourceGroups/test-resource-group\"}}}'\n template: '{}'\n"
actual : "apiVersion: radapp.io/v1alpha3\nkind: DeploymentTemplate\nmetadata:\n name: azure.yaml\n namespace: radius-system\nspec:\n parameters: '{}'\n providerConfig: '{\"radius\":{\"type\":\"radius\",\"value\":{\"scope\":\"/planes/radius/local/resourceGroups/test-resource-group\"}},\"az\":{\"type\":\"azure\",\"value\":{\"scope\":\"/subscriptions/test-subId/resourceGroups/test-rg\"}},\"deployments\":{\"type\":\"Microsoft.Resources\",\"value\":{\"scope\":\"/planes/radius/local/resourceGroups/test-resource-group\"}}}'\n repository: azure.yaml\n template: '{}'\n"
Diff:
--- Expected
+++ Actual
@@ -8,2 +8,3 @@
providerConfig: '{"radius":{"type":"radius","value":{"scope":"/planes/radius/local/resourceGroups/test-resource-group"}},"az":{"type":"azure","value":{"scope":"/subscriptions/test-subId/resourceGroups/test-rg"}},"deployments":{"type":"Microsoft.Resources","value":{"scope":"/planes/radius/local/resourceGroups/test-resource-group"}}}'
+ repository: azure.yaml
template: '{}'
Test: Test_Run/Create_DeploymentTemplate_with_Azure_scope
--- FAIL: Test_Run/Create_DeploymentTemplate_with_Azure_scope (0.00s)
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.
Running make generate
results in these files being changed.
deploy/Chart/crds/radius/radapp.io_deploymentresources.yaml
deploy/Chart/crds/radius/radapp.io_deploymenttemplates.yaml
deploy/Chart/crds/radius/radapp.io_recipes.yaml
deploy/Chart/crds/ucpd/ucp.dev_queuemessages.yaml
deploy/Chart/crds/ucpd/ucp.dev_resources.yaml
For example, controller-gen.kubebuilder.io/version: v0.16.4
changes to controller-gen.kubebuilder.io/version: v0.16.0
Signed-off-by: willdavsmith <[email protected]>
Signed-off-by: willdavsmith <[email protected]>
Signed-off-by: willdavsmith <[email protected]>
Signed-off-by: willdavsmith <[email protected]>
Signed-off-by: willdavsmith <[email protected]>
Signed-off-by: willdavsmith <[email protected]>
Signed-off-by: willdavsmith <[email protected]>
Signed-off-by: willdavsmith <[email protected]>
Signed-off-by: willdavsmith <[email protected]>
Signed-off-by: willdavsmith <[email protected]>
Signed-off-by: willdavsmith <[email protected]>
Signed-off-by: willdavsmith <[email protected]>
Signed-off-by: willdavsmith <[email protected]>
Radius functional test overview
Click here to see the list of tools in the current test run
Test Status⌛ Building Radius and pushing container images for functional tests... |
Signed-off-by: willdavsmith <[email protected]>
Signed-off-by: willdavsmith <[email protected]>
commonflags.AddResourceGroupFlag(cmd) | ||
commonflags.AddParameterFlag(cmd) | ||
|
||
cmd.Flags().StringP("destination-file", "d", "", "Path of the generated DeploymentTemplate yaml file.") |
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.
Is this the file to be generated by the command? Should it be existing before running the command? The description may need an update.
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.
updated the description: "Path of the generated DeploymentTemplate yaml file created by running this command."
AWSScope string | ||
} | ||
|
||
// NewRunner creates a new instance of the `rad deploy` runner. |
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.
needs an update
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.
fixed
r.DestinationFile = strings.TrimSuffix(filepath.Base(r.FilePath), filepath.Ext(r.FilePath)) + ".yaml" | ||
} | ||
|
||
if filepath.Ext(r.DestinationFile) != ".yaml" { |
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.
Would this be a problem if some provides .yml
as the extension?
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.
updated to accept both
return err | ||
} | ||
|
||
// Print the path to the file |
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.
Do we need this comment?
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.
removed
var buf bytes.Buffer | ||
encoder := yaml.NewEncoder(&buf) | ||
|
||
// Set the indentation to 2 spaces |
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.
nit: not necessary because code is clear enough
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.
fixed
}, | ||
} | ||
} | ||
if r.Group != "" { |
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.
If group is empty at this point, then we have a problem, right? I mean according to the code this case shouldn't happen but just asking.
}, | ||
{ | ||
Name: "rad bicep generate-kubernetes-manifest - valid with group long flag", | ||
Input: []string{"app.bicep", "--group", "default"}, |
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.
No group should also be tested.
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.
added the test
parser := bicep.ParameterParser{FileSystem: bicep.OSFileSystem{}} | ||
parser := bicep.ParameterParser{FileSystem: filesystem.NewOSFS()} |
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.
What is the main reason of this change?
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.
I needed a filesystem abstraction that handles CreateFile
since rad bicep generate-kubernetes-manifest
creates a file. Instead of using a library, I wrote a simple one. See more info here: #8001 (comment)
|
||
_, _ = fs.Create(fileName) | ||
|
||
file, err := fs.Open(fileName) |
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.
Do any of the functions in the MemMapFileSystem need a defer close
or something like that when called?
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.
no, because they aren't actually opening a file, they're just adding content to a map
in memory. This is useful for testing
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.
Is this supposed to have unit tests?
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.
These are just a wrapper over the os
functions so I don't think unit tests are necessary.
Radius functional test overview
Click here to see the list of tools in the current test run
Test Status⌛ Building Radius and pushing container images for functional tests... |
return &OSFileSystem{} | ||
} | ||
|
||
func (osfs OSFileSystem) Create(name string) (fs.File, error) { |
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.
Should we have unit testing for this abstraction? There is testing forMemMapFS
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.
These are just a wrapper over the os functions so I don't think unit tests are necessary.
"io/fs" | ||
) | ||
|
||
// FileSystem is an interface that defines the methods needed to interact with a file system. |
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.
Could you explain the use cases for OSFS and MemMapFS? It seems like MemMapFS is only used for testing while OSFS is used when generating the manifest files. Is MemMapFS used to make testing the generate manifest code easier?
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.
That's correct. In the real world (when a user runs the command), they expect to have the file generated for them on their OS filesystem (so we use OSFS). Since we don't want to have a whole OS filesystem for unit tests we can just use the MemMapFS.
} | ||
|
||
func waitForDeploymentTemplateReady(t *testing.T, ctx context.Context, name types.NamespacedName, client controller_runtime.WithWatch, initialVersion string) (*radappiov1alpha3.DeploymentTemplate, error) { | ||
// Based on https://gist.github.com/PrasadG193/52faed6499d2ec739f9630b9d044ffdc |
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.
Can you add comments for what this code does
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.
done
controller_runtime "sigs.k8s.io/controller-runtime/pkg/client" | ||
) | ||
|
||
func Test_DeploymentTemplate_Basic(t *testing.T) { |
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.
func Test_DeploymentTemplate_Basic(t *testing.T) { | |
func Test_DeploymentTemplate_Env(t *testing.T) { |
so it lines up with the file name?
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.
done
func createEnvironment(radius *mockRadiusClient, name string) { | ||
id := fmt.Sprintf("/planes/radius/local/resourceGroups/%s/providers/Applications.Core/environments/%s", name, name) | ||
func createEnvironment(radius *mockRadiusClient, resourceGroup, name string) { | ||
id := fmt.Sprintf("/planes/radius/local/resourceGroups/%s/providers/Applications.Core/environments/%s", resourceGroup, name) |
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.
Just confirming, did we add the RG because we create one if it doesn't exist as part of the deployment template controller?
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.
yep, also just for clarity
"resourceId": "self", | ||
"namespace": "default" | ||
} | ||
} |
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.
Do we need to test for different provider configs?
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.
for now I think this is okay. it'll take some work to add these tests, we can do it later.
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.
This PR is already large enough :)
existingOutputResources[resource] = true | ||
} | ||
|
||
newOutputResources := make(map[string]bool) |
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.
I did not see a test for this functionality (could it be pulled out)?
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.
added a test
@@ -0,0 +1,393 @@ | |||
/* | |||
Copyright 2024 The Radius Authors. |
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.
Some files have this as 2024 while most have 2023. May not be of consequence but good to be consistent.
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.
I'll just put 2024 for now since this is the same as the other reconciler files
@@ -0,0 +1,15 @@ | |||
package filesystem |
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.
Missing copyright text in some new files
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.
fixed
Is it possible to have a multi-step test where where we deploy a template file and check it's updated and then we deploy an updated file for the same template with updated resources and check for correct update? That may be a way to test the updates to outputresources unless there was another way that exists and I missed it. |
Signed-off-by: willdavsmith <[email protected]>
yep, just added one! good callout |
Signed-off-by: willdavsmith <[email protected]>
Radius functional test overview
Click here to see the list of tools in the current test run
Test Status⌛ Building Radius and pushing container images for functional tests... |
Description
rad bicep generate-kubernetes-manifest
CLIType of change
#6689
Contributor checklist
Please verify that the PR meets the following requirements, where applicable: