-
Notifications
You must be signed in to change notification settings - Fork 473
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
Remove Windows MDM feature flag #15167
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #15167 +/- ##
==========================================
- Coverage 67.50% 67.49% -0.01%
==========================================
Files 1030 1030
Lines 89619 89669 +50
Branches 2306 2302 -4
==========================================
+ Hits 60493 60519 +26
- Misses 24622 24640 +18
- Partials 4504 4510 +6
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
de91b98
to
262cc20
Compare
is released. the view all UI will show in the windows column when we | ||
release the feature. */} | ||
{!original.includeWindows && ( | ||
{!true && ( |
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 here I'm really not sure what to do? By removing mdm_enabled
, includeWindows
is now always true
, so that means I should delete the whole {...}
part? So <ViewAllHostsLink>
would not be rendered anymore, does that sound correct?
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.
We want View all hosts to always appear now so I'm gonna remove the conditional and just render that component
@@ -234,11 +234,11 @@ const AppleBusinessManagerSection = ({ | |||
<div className={baseClass}> | |||
<h2>Apple Business Manager</h2> | |||
{isLoadingMdmAppleBm ? <Spinner /> : renderAppleBMInfo()} | |||
{config?.mdm_enabled && ( | |||
{ |
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.
Should I remove those {}
around <WindowsAutomaticEnrollmentCard>
?
<WindowsMdmCard | ||
turnOnWindowsMdm={navigateToWindowsMdm} | ||
editWindowsMdm={navigateToWindowsMdm} | ||
/> |
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.
Or should I have kept the {}
around <WindowsMdmCard>
?
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.
without the {}
looks good to me! (the {}
are used to wrap expressions)
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.
Thanks, I'll remove them from here then.
@@ -1,8 +1,6 @@ | |||
output "extra_environment_variables" { | |||
value = merge(var.enable_apple_mdm == false ? {} : { | |||
FLEET_MDM_APPLE_SERVER_ADDRESS = var.public_domain_name | |||
}, var.enable_windows_mdm == false ? {} : { | |||
FLEET_DEV_MDM_ENABLED = "1" |
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.
For infra folks reviewing this - I removed the feature flag environment variable but left everything else intact in the terraform, this means the var.enable_windows_mdm
variable is still there (as I think it should, since it is used to define the WSTEP cert or not). TBD if the default value should now be set to true
as it is for Apple?
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.
It'd be safe to do so either way because having the WSTEP cert bytes populated by the SCEP key/cert doesn't change anything until they add check they want Windows MDM in the UI. Setting the default to enabled would make it fairly easy to allow existing Mac MDM enjoyers to have the Windows needed-configurations in place right away.
Regardless I'm happy to approve from the Infra side either way.
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.
Thanks @rfairburn , in order to keep the impact of this PR as focused as possible I'll leave it as it is for now.
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.
Approval is for infra/terraform portion of the changes.
For #14959 , but I believe it also requires frontend changes.
Checklist for submitter
changes/
ororbit/changes/
.See Changes files for more information.