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

chore(scaleway-variant): add lb annotation #79

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

Xaving
Copy link
Member

@Xaving Xaving commented Jun 19, 2024

Use scaleway annotation for load balancer.

The change only impacts scaleway part.

Breaking change

  • No
  • Yes (in the Helm chart(s)): indicate the chart, version & release note link
  • Yes (in the module itself): give an explanation of the breaking change

Tests executed on which distribution(s)

  • KinD
  • AKS (Azure)
  • EKS (AWS)
  • Scaleway
  • SKS (Exoscale)

@Xaving Xaving requested a review from a team as a code owner June 19, 2024 08:44
Copy link
Contributor

@lentidas lentidas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the scope of the commit and the title of the PR I would put scaleway instead of scaleway-variant because variant is implied.

helm_values = [{
traefik = {
service = {
type = "LoadBalancer"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since you're using a service type of LoadBalancer, on the main.tf you do not need to call the nodeport variant but instead you can call the main module directly. So the source in this line should be ../ instead of ../nodeport.

Comment on lines +2 to +3
type = string
description = "ID of the main load balancer"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For a question of maintaining style with the other modules, the description should come first. I also added a nullable = false because I think this value is required and we cannot risk anybody passing a null value and cause an undefined behavior (that is something I also should have done on the SKS variant and did not do it).

Suggested change
type = string
description = "ID of the main load balancer"
description = "ID of the Scaleway Load Balancer to use for the Kubernetes cluster."
type = string
nullable = false

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.

2 participants