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

ContainerLogsV2 and Basic Logs support #492

Merged
merged 19 commits into from
Jan 17, 2023
Merged

ContainerLogsV2 and Basic Logs support #492

merged 19 commits into from
Jan 17, 2023

Conversation

samaea
Copy link
Contributor

@samaea samaea commented Jan 6, 2023

PR Summary

Addresses issue #437.

In addition, added:-

  • updateDependencyFn() function (under addonsTabs) that assists with enforcing dependency between two features (e.g. In order to activate Basic Logs on Container Logs V2, you also need to enable the Container Logs V2 schema. It might be worth considering to make this a global function instead (i.e same place as where updateFn() is defined).
  • Under deployTabs, preview_post_params array has been added to support flagging preview features passed as params to the postdeploy script. At present, we are only capturing preview params under ARM/Bicep.

image

image

PR Checklist

  • PR has a meaningful title
  • Summarized changes
  • This PR is ready to merge and is not Work in Progress
  • Link to a filed issue
  • Screenshot of UI changes (if PR includes UI changes)

bicep/main.bicep Show resolved Hide resolved
postdeploy/scripts/certmanager-install.sh Outdated Show resolved Hide resolved
@Gordonby Gordonby requested a review from khowling January 6, 2023 14:48
@Gordonby Gordonby added enhancement New feature or request azure-preview-feature References a preview feature labels Jan 6, 2023
@samaea samaea force-pushed the samaea-containerlogsv2 branch from 93c5868 to 993e56e Compare January 7, 2023 22:58
@samaea
Copy link
Contributor Author

samaea commented Jan 15, 2023

👋 @khowling, incorporated feedback from our PR discussion. Also added another function in addons.js to ensure any features dependent on the ContainerLogsV2 schema are disabled as well (at present it only accommodates for the Basic Logs feature). This avoids the scenario where the user selects on Basic Logs (which automatically selects the ContainerLogsV2 feature) and then decides to disable the ContainerLogsV2 feature (i.e safety guard in both directions now).

@khowling
Copy link
Contributor

hey @samaea , looking good! I just tested the UI, and one small thing, in Monitoring, If I select 'Azure Monitor for Containers', then select the V2 Logs, the parameters appear on the deployment command as expected. But it I then select 'None' for Monitoring, the V2 log parameters still appear. To fix this, can you add an additional check in the deployment logic, take a look at this as an example
image

@samaea
Copy link
Contributor Author

samaea commented Jan 16, 2023

hey @samaea , looking good! I just tested the UI, and one small thing, in Monitoring, If I select 'Azure Monitor for Containers', then select the V2 Logs, the parameters appear on the deployment command as expected. But it I then select 'None' for Monitoring, the V2 log parameters still appear. To fix this, can you add an additional check in the deployment logic, take a look at this as an example image

Hey @khowling - great catch! All done. :)

@samaea samaea merged commit 366608f into main Jan 17, 2023
@samaea samaea deleted the samaea-containerlogsv2 branch January 17, 2023 16:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
azure-preview-feature References a preview feature enhancement New feature or request helper-ui
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants