Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Load Balancer: Add better support for privateLB #1089

Merged
merged 27 commits into from
Mar 8, 2022

Conversation

MariusStorhaug
Copy link
Contributor

@MariusStorhaug MariusStorhaug commented Mar 5, 2022

Change

Minor changes to have the LB be able to be used in a solution deployment. It allows a LB be deployed first, having VMs be deployed after to be put into the backendpool of the LB.

Also added a test for creating a private LB.

Network: LoadBalancers

Type of Change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update (Wiki)

Checklist

  • I'm sure there are no other open Pull Requests for the same update/change
  • My corresponding pipelines / checks run clean and green without any errors or warnings
  • My code follows the style guidelines of this project
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation (readme)
  • I did format my code

@github-actions
Copy link

github-actions bot commented Mar 5, 2022

Unit Test Results

    1 files  1 suites   47s ⏱️
    8 tests 8 ✔️   0 💤 0
104 runs  8 ✔️ 96 💤 0

Results for commit 85372cf.

♻️ This comment has been updated with latest results.

@MariusStorhaug MariusStorhaug marked this pull request as ready for review March 5, 2022 22:42
@MariusStorhaug MariusStorhaug enabled auto-merge (squash) March 5, 2022 22:48
Marius Storhaug and others added 8 commits March 6, 2022 11:01
…eters.json

Co-authored-by: Erika Gressi <56914614+eriqua@users.noreply.github.com>
…rs/parameters/internal.parameters.json

Co-authored-by: Erika Gressi <56914614+eriqua@users.noreply.github.com>
Co-authored-by: Erika Gressi <56914614+eriqua@users.noreply.github.com>
Co-authored-by: Erika Gressi <56914614+eriqua@users.noreply.github.com>
Co-authored-by: Erika Gressi <56914614+eriqua@users.noreply.github.com>
Co-authored-by: Erika Gressi <56914614+eriqua@users.noreply.github.com>
@MariusStorhaug MariusStorhaug requested a review from eriqua March 6, 2022 10:08
@MariusStorhaug
Copy link
Contributor Author

@MrMCake @eriqua ready for a new look. Sorted the comments

@MariusStorhaug MariusStorhaug requested a review from rahalan March 6, 2022 22:49
Copy link
Contributor

@ahmadabdalla ahmadabdalla left a comment

Choose a reason for hiding this comment

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

Should we distinguish the parameter names as:

  • 'external.min.parameters.json'
  • 'internal.min.parameters.json' (not there yet)
  • 'external.parameters.json'
  • 'internal.parameters.json'

Similar to the VM module?
image

@AlexanderSehr
Copy link
Contributor

Should we distinguish the parameter names as:

  • 'external.min.parameters.json'
  • 'internal.min.parameters.json' (not there yet)
  • 'external.parameters.json'
  • 'internal.parameters.json'

Similar to the VM module? image

@ahmadabdalla would there be a use case for that? In case of the VM the resources & conditions that are deployed differ quite a bit from windows to linux. Is the same true for LB?

@ahmadabdalla
Copy link
Contributor

Not a use case more of one supporting a public ip and one doesn't. If we are introducing a new type, maybe the parameter names should align.

Plus there is a new sku of 'gateway' and tier of 'global' which we are yet to support. This may introduce different properties combinations so we would end up with different files.

@MariusStorhaug
Copy link
Contributor Author

MariusStorhaug commented Mar 8, 2022

@ahmadabdalla and @MrMCake: I don't see the need to introduce this now as there is currently no use case for it. We can add it 'when we get there' as its a change that would not affect the module validation or alignment at this point. Just a "lean" thought from my end on this issue. Lets not overthink it at this point.

@MariusStorhaug MariusStorhaug merged commit 0dc44cc into main Mar 8, 2022
@MariusStorhaug MariusStorhaug deleted the users/mast/LB-privateLB branch March 8, 2022 12:08
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants