-
Notifications
You must be signed in to change notification settings - Fork 102
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 Radius Flux Controller for GitOps support #8784
base: main
Are you sure you want to change the base?
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.
Awesome PR 🚀
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 pressed the button on the previous review before finished. Here is part 2. I'm still reviewing so I will add more in another section.
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.
First round of my review.
nodes: | ||
- role: control-plane | ||
extraPortMappings: | ||
- containerPort: 30080 | ||
hostPort: 30080 | ||
protocol: TCP |
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 reason behind this addition? Do we need to add some comments here?
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 is for Gitea. I added a comment
service: | ||
http: | ||
type: NodePort | ||
nodePort: 30080 | ||
ssh: | ||
type: ClusterIP | ||
port: 22 |
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 it is kind of making sense what you did in the kind cluster creation action. But we may still need to add some comments to the action so that people can understand what it is for.
fi | ||
|
||
if [ -z "$GITEA_ACCESS_TOKEN_NAME" ]; then | ||
echo |
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 should be removed
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 mean the empty echo
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
helm repo add gitea-charts https://dl.gitea.io/charts/ | ||
helm repo 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.
Are we always going to install the latest version? Can it cause any breaking changes at some point?
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.
true - changed this to install 11.0.0
output=$(kubectl exec -it $gitea_pod -n gitea -- gitea admin user generate-access-token --username $GITEA_USERNAME --token-name $GITEA_ACCESS_TOKEN_NAME --scopes "write:repository,write:user" --raw) | ||
echo $output | ||
|
||
echo "gitea-access-token=$output" >>$GITHUB_OUTPUT |
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 sh file can actually be tested locally.
deploy/images/controller/Dockerfile
Outdated
@@ -1,17 +1,20 @@ | |||
# Use distroless image which already includes ca-certificates | |||
FROM gcr.io/distroless/static:nonroot | |||
FROM ubuntu:latest |
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.
- We should check the size difference since we are changing the base image (I believe ubuntu is probably much larger than what we have now)
- We should also lock the version to not have breaking changes
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 this to use debian-slim
and locked the version.
pkg/cli/bicep/types.go
Outdated
@@ -38,12 +41,13 @@ var _ Interface = (*Impl)(nil) | |||
|
|||
// Impl is the implementation of Interface. | |||
type Impl struct { | |||
filesystem filesystem.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.
Should this be 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.
yes, updated
pkg/cli/bicep/types.go
Outdated
// Build runs `rad-bicep build` with the given arguments. | ||
func (i *Impl) Build(args ...string) (map[string]any, error) { | ||
buildArgs := make([]string, len(args)+1) | ||
buildArgs[0] = "build" | ||
copy(buildArgs[1:], args) | ||
|
||
return runBicepJSON(buildArgs...) | ||
} | ||
|
||
// BuildParams runs `rad-bicep build-params` with the given arguments. | ||
func (i *Impl) BuildParams(args ...string) (map[string]any, error) { | ||
buildParamsArgs := make([]string, len(args)+1) | ||
buildParamsArgs[0] = "build-params" | ||
copy(buildParamsArgs[1:], args) | ||
|
||
return runBicepJSON(buildParamsArgs...) | ||
} |
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 could be one generic function with the initial param passed in like: BuildCommand("build-params" or "build", args).
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 deleted these functions and just wrote Call()
instead
pkg/cli/bicep/types.go
Outdated
|
||
version := regexp.MustCompile(SemanticVersionRegex).FindString(string(bytes)) | ||
if version == "" { | ||
return fmt.Sprintf("unknown (failed to parse bicep version from %q)", string(bytes)) |
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.
Shouldn't this also return an error? Like func (i *Impl) Version() (string, 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.
this code is copied over from pkg/cli/bicep/build.go - I don't want to change it in case it breaks something
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 all the changes in this PR strictly dependent on one another? Ideally, we should aim to break them down into smaller, more focused PRs, keeping each one as minimal as possible for easier review.
deploy/images/bicep/Dockerfile
Outdated
@@ -0,0 +1,14 @@ | |||
FROM alpine:latest |
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 want to pin this to a specific version and make it more deterministic
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 call - changing to 3.21.3
deploy/images/controller/Dockerfile
Outdated
# Use Ubuntu image to enable Bicep CLI. | ||
# Switch to something more lightweight when we can find a | ||
# base image that supports running Bicep. | ||
FROM ubuntu:latest |
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.
Same as the above comment of pinning to a specific version instead of latest
pkg/cli/bicep/types.go
Outdated
|
||
// Version returns the version of Bicep installed on the local machine, | ||
// or an error if Bicep cannot be found or is not a valid version. | ||
func (i *Impl) Version() string { |
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 think we should return the version or an error and not co-mingle the return
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 code is copied over from pkg/cli/bicep/build.go - I don't want to change it in case it breaks something
file, exists := m.InternalFileSystem[name] | ||
if !exists { | ||
return nil, fmt.Errorf("file %s does not exist", 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.
nit: newline
_, exists := m.InternalFileSystem[name] | ||
if !exists { | ||
return fmt.Errorf("file %s does not exist", 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.
nit: newline
0b2241a
to
90e98b3
Compare
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]>
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.
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.
🚢
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... |
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
controller
podNote: I expect the functional tests to fail for this PR because I'm updating the functional test workflow. Here's a link to a successful action run: https://github.com/radius-project/radius/actions/runs/14074009492
Design PR: radius-project/design-notes#79
Docs PR: radius-project/docs#1408
Type of change
Fixes: #6689
Contributor checklist
Please verify that the PR meets the following requirements, where applicable: