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
fix:
avm/res/web/static-site
fixes for pdns & Identity Var #4693base: main
Are you sure you want to change the base?
fix:
avm/res/web/static-site
fixes for pdns & Identity Var #4693Changes from 81 commits
7ecd1a5
0f15c86
c6993a6
0e65d72
482add6
1bffa61
8c358e7
14a5d0d
fba5375
05152a8
f8d04c6
d22a6c7
6993c4d
cf44304
5260056
3740075
c9b6b4c
a3d83a4
915d12c
1c56bb3
454bcc5
fe017c1
ddb6e04
1931d9a
657311c
fff3279
c5f6e4f
ffc5837
e9b2e80
018c016
9a3e2bc
ebed1d1
d587154
18a9a54
bb9037e
de4e665
8994eff
88df6f6
cdfb68d
0e3020f
40614c1
4240d6c
6e16d58
d41dec0
689f23d
6cd9703
27de0ec
3249154
65802d0
8a0d70b
e19c970
dc5e12d
379534f
77e2f8c
6e0b216
e4cf2a0
bacee7d
e8c9b85
4ca1290
3211b7c
bb507f9
7ec58b0
88d09f1
0a7cefc
64d7206
ff6facc
d0dadd0
77014ff
c679dd1
3dff828
6c9b47e
6579a77
783a91c
739c32a
abb4049
af4846a
ce51501
75cc178
0bd8388
4bb9bbb
5010596
f2e02bb
4e5d33b
bca5c4b
36efa6e
01c8b4d
b7a532f
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.
Considering the amount of properties one is able to configure in the module I wonder if we may need to introduce a parameterObject for them - like done e.g. for the
nicConfigurations
parameter in the VM module.Just thinking out loud here. If you cannot configure it to you needs you're essentially required to idempotently redeploy the DNS Zone after the deployment with another deployment. Maybe that's ok and intended - but wanted to raise the thought :)
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.
So on this thought, for a VM the NIC is required, I guess here we do follow a smiliar pattern except the PDNS Zone is only needed for private endpoints to work. Typically the DNS Zone should sit with the rest of the DNS Zone and be configured with a vnet link.
In this current implemntation the app will still not be routable as the DNS Zone has not been linked to a Virtual Network, I need to update this so it accepts some properties. My view is this should be the bare minimal requirements to ensure PE connectivity is established and routable but any extensive configuration should be done in a dedicated module for the staticSite zone. il make an update to this today
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.
Additional param added for virtualNetworkResourceId
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 wonder if that's something we should chat about some time - just to ensure we're all on the same page. The next maintainer call would come to mind 💪
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 looks like the change is still tested. Should the PR be in draft?
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, I just hadn't removed this, my bad
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.
udpated