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.

Sign up for GitHub

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

Jump to bottom

[Fleet] Fix missing package-level vars in GCP policy editor #132068

Merged
merged 5 commits into from
May 16, 2022
Merged

[Fleet] Fix missing package-level vars in GCP policy editor #132068

merged 5 commits into from
May 16, 2022

Conversation

kpollich
Copy link
Member

Summary

Display package-level variables even if they aren't defined on the existing package policy during an upgrade, honoring default values for all variables if they exist.

Fixes #131251

Testing

See #131251 (comment) for detailed testing instructions. You'll need a copy of the WIP gcp-2.0.0 integration attached to that issue, and a local package registry instance serving that integration.

Screen Recording

Recording demonstrates proper rendering of package-level variables when upgrading from 1.5.0 -> 2.0.0 for the gcp integration. Previously, these package-level variables were not rendered in the UI.

Screen.Recording.2022-05-11.at.4.02.17.PM.mov

@kpollich
Fix missing package-level vars in GCP policy editor
132f547 
Display package-level variables even if they aren't defined on the
existing package policy during an upgrade, honoring default values for
all variables if they exist.

Fixes elastic#131251
@kpollich kpollich added release_note:skip Skip the PR/issue when compiling release notes Team:Fleet Team label for Observability Data Collection Fleet team v8.3.0 labels May 11, 2022
@kpollich kpollich requested a review from a team as a code owner May 11, 2022 20:17
@kpollich kpollich self-assigned this May 11, 2022
@elasticmachine
Copy link
Contributor

Pinging @elastic/fleet (Team:Fleet)

nchaulet
nchaulet reviewed May 11, 2022
View reviewed changes
...ations/fleet/sections/agent_policy/create_package_policy_page/step_define_package_policy.tsx Outdated
if (!packagePolicy.vars || !packagePolicy.vars[varName]) return null;
const value = packagePolicy.vars[varName].value;
const { name: varName, type: varType, default: defaultValue } = varDef;
const value = packagePolicy.vars?.[varName]?.value ?? defaultValue;
Copy link
Member

Choose a reason for hiding this comment

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

Should we populate packagePolicy.vars?.[varName]?.value with the default value here instead of displaying the default value, I think if the user do not change the value it will not be saved no? (not tested locally)

Copy link
Member Author

Choose a reason for hiding this comment

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

I just pushed up fede140 which should address this. We now create the vars[varName] object entry on packagePolicy and set the default value if necessary, and the values/vars persist as expected.

image

nchaulet
nchaulet reviewed May 12, 2022
View reviewed changes
...ations/fleet/sections/agent_policy/create_package_policy_page/step_define_package_policy.tsx Outdated
const value = packagePolicy.vars?.[varName]?.value ?? defaultValue;

// Set up the `vars` object and assign the default value if it doesn't exist
if (!packagePolicy.vars) {
Copy link
Member

Choose a reason for hiding this comment

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

it seems odd to me to reassign vars in a component render, can we do this in a useEffect calling onChange ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Moved this logic to the existing useEffect and verified things still work in 2e2469e.

Also added tests.

@kpollich
Copy link
Member Author

@elasticmachine merge upstream

@kpollich kpollich enabled auto-merge (squash) May 16, 2022 12:18
@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
fleet 704.6KB 704.7KB +108.0B

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
fleet 90.9KB 90.9KB +8.0B

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

cc @kpollich

@kpollich kpollich merged commit 5fff510 into elastic:main May 16, 2022
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label May 16, 2022
@kpollich kpollich deleted the 131251-fix-gcp-policy-editor branch May 16, 2022 13:35
endorama added a commit to elastic/integrations that referenced this pull request Jul 12, 2022
@endorama
update kibana constraint
edc3905 
Before version 8.3.0 a bug in the Fleet UI configuration conflict
resolution screen would prevent a successful update of this
package to the new version.

The constraint is updated on Kibana >= 8.3 where the related fix
landed[1] so we can guarantee a working upgrade path.

[1]: elastic/kibana#132068
endorama added a commit to elastic/integrations that referenced this pull request Jul 13, 2022
@endorama
update kibana constraint
826b985 
Before version 8.3.0 a bug in the Fleet UI configuration conflict
resolution screen would prevent a successful update of this
package to the new version.

The constraint is updated on Kibana >= 8.3 where the related fix
landed[1] so we can guarantee a working upgrade path.

[1]: elastic/kibana#132068
@kpollich kpollich added auto-backport Deprecated - use backport:version if exact versions are needed v7.17.6 and removed backport:skip This commit does not require backporting labels Jul 13, 2022
@kibanamachine
Copy link
Contributor

💔 All backports failed

Status Branch Result
❌ 7.17 Backport failed because of merge conflicts

You might need to backport the following PRs to 7.17:
- revert package policy validation that caused issue with input groups (#125657)
- [Fleet] fixed undefined error when missing policy vars in template (#124215)

Manual backport

To create the backport manually run:

node scripts/backport --pr 132068

Questions ?

Please refer to the Backport tool documentation

@kpollich kpollich mentioned this pull request Jul 13, 2022
Merged
kpollich added a commit to kpollich/kibana that referenced this pull request Jul 13, 2022
@kpollich
[Fleet] Fix missing package-level vars in GCP policy editor (elastic#…
b3ae98e 
…132068)

* Fix missing package-level vars in GCP policy editor

Display package-level variables even if they aren't defined on the
existing package policy during an upgrade, honoring default values for
all variables if they exist.

Fixes elastic#131251

* Fix not persisting default values for new variables

* Fix tests

* Address PR feedback + update tests

(cherry picked from commit 5fff510)

# Conflicts:
#	x-pack/plugins/fleet/common/services/validate_package_policy.test.ts
#	x-pack/plugins/fleet/public/applications/fleet/sections/agent_policy/create_package_policy_page/step_define_package_policy.test.tsx
@endorama endorama mentioned this pull request Jul 14, 2022
Closed
@kibanamachine
Copy link
Contributor

Looks like this PR has a backport PR but it still hasn't been merged. Please merge it ASAP to keep the branches relatively in sync.

@kibanamachine kibanamachine added backport missing Added to PRs automatically when the are determined to be missing a backport. and removed backport missing Added to PRs automatically when the are determined to be missing a backport. labels Jul 14, 2022
kpollich added a commit that referenced this pull request Jul 14, 2022
@kpollich @kibanamachine
[7.17] [Fleet] Fix missing package-level vars in GCP policy editor (#…
bbb1021 
…132068) (#136289)

* [Fleet] Fix missing package-level vars in GCP policy editor (#132068)

* Fix missing package-level vars in GCP policy editor

Display package-level variables even if they aren't defined on the
existing package policy during an upgrade, honoring default values for
all variables if they exist.

Fixes #131251

* Fix not persisting default values for new variables

* Fix tests

* Address PR feedback + update tests

(cherry picked from commit 5fff510)

# Conflicts:
#	x-pack/plugins/fleet/common/services/validate_package_policy.test.ts
#	x-pack/plugins/fleet/public/applications/fleet/sections/agent_policy/create_package_policy_page/step_define_package_policy.test.tsx

* Remove incorrect test

* Remove define package policy tests from 7.17.x

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
@kpollich
Copy link
Member Author

@endorama - Just finished backporting this (had some dependent PR's) to 7.17. Let me know if any issues arise. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nchaulet nchaulet nchaulet approved these changes

Assignees

@kpollich kpollich

Labels
auto-backport Deprecated - use backport:version if exact versions are needed release_note:skip Skip the PR/issue when compiling release notes Team:Fleet Team label for Observability Data Collection Fleet team v7.17.6 v8.3.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Policy fields disappearing during upgrade process with conflict resolution
5 participants
@kpollich @elasticmachine @kibana-ci @kibanamachine @nchaulet