Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Adding Postgres Helm chart to rad init #8072
base: main
Are you sure you want to change the base?
Adding Postgres Helm chart to rad init #8072
Changes from all commits
e89325d
dec32fa
a131ba3
69e3fc8
97e6a52
08b4387
33a5958
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 need to change depending on where the Kubernetes cluster is hosted?
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.
Curious how did we decide these values. Would helpful if you could add a comment in the code for context as well.
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 add a comment 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.
are we adding this to resolve the sqlserver functional test issues?
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 was an attempt to try and limit the size of the sqlserver, I'll revert in a subsequent commit
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 a pretty large amount of resources still (2 GB). Did this help?
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 per our offline discussion, do we want to go ahead and delete this test? If so, @superbeeny can do this as part of this PR which will also unblock it.
For context, @superbeeny, we added a functional test for every portable resource type initially because we had custom implementation for each of them. Since then the implementation has evolved and majority of the code has converged across all types, so it is covered extensively through other functional tests. SQL server specifically is also covered in Radius eshop sample.
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.
+1 to the consensus that this is ok to remove. It think that would mean deleting https://github.com/radius-project/radius/blob/main/test/functional-portable/datastoresrp/noncloud/resources/sql_test.go and the other files it references.
There's actually more than one sql server test which is why I think we're hitting issues. That's 4gb minimum used by those tests.