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

[Fleet] Bulk upgrade api response change #95236

Merged
merged 13 commits into from
Mar 26, 2021

Conversation

jfsiii
Copy link
Contributor

@jfsiii jfsiii commented Mar 23, 2021

Summary

/agents/bulk_upgrade should return a response with a result for each agent given; including invalid or missing ids. It currently returns an empty object.

This PR includes commits from open PR #95024. The additions from this PR are https://github.com/jfsiii/kibana/compare/bulk-reassign-response-should-include-all-given-agents..871ebcb

TS type diff for response

- // eslint-disable-next-line @typescript-eslint/no-empty-interface
- export interface PostBulkAgentUpgradeResponse {}

+ export type PostBulkAgentUpgradeResponse = Record<
+   Agent['id'],
+   {
+     success: boolean;
+     error?: string;
+   }
+ >;

Checklist

@jfsiii jfsiii changed the title Bulk upgrade api response change [Fleet] Bulk upgrade api response change Mar 23, 2021
@jfsiii jfsiii marked this pull request as ready for review March 24, 2021 01:17
@jfsiii jfsiii requested a review from a team as a code owner March 24, 2021 01:17
@jfsiii jfsiii self-assigned this Mar 24, 2021
@jfsiii jfsiii added release_note:skip Skip the PR/issue when compiling release notes Team:Fleet Team label for Observability Data Collection Fleet team v7.13.0 labels Mar 24, 2021
@elasticmachine
Copy link
Contributor

Pinging @elastic/fleet (Team:Fleet)

@jfsiii jfsiii added the v8.0.0 label Mar 24, 2021
@jfsiii jfsiii requested review from nchaulet and jen-huang March 24, 2021 14:13
Copy link
Contributor

@Zacqary Zacqary left a comment

Choose a reason for hiding this comment

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

Code looks good, just a few string literals in error messages that are missing i18n.

for (const agentResult of givenAgentsResults) {
if (agentResult.found === false) {
outgoingErrors[agentResult._id] = new AgentReassignmentError(
`Cannot find agent ${agentResult._id}`
Copy link
Contributor

Choose a reason for hiding this comment

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

This error message needs to be wrapped in i18n.translate

if (policy.is_managed) {
throw new IngestManagerError(`Cannot upgrade agent in managed policy ${policy.id}`);
if (!options.force && agent.policy_id && managedPolicies[agent.policy_id]) {
throw new IngestManagerError(`Cannot upgrade agent in managed policy ${agent.policy_id}`);
Copy link
Contributor

Choose a reason for hiding this comment

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

Needs i18n.translate

givenAgents.map(async (agent) => {
const isAllowed = options.force || isAgentUpgradeable(agent, kibanaVersion);
if (!isAllowed) {
throw new IngestManagerError(`${agent.id} is not upgradeable`);
Copy link
Contributor

Choose a reason for hiding this comment

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

Needs i18n.translate

Copy link
Member

@nchaulet nchaulet left a comment

Choose a reason for hiding this comment

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

Tested and work as expected 👍
For translating the errors message, we did not have any error message translated in the Fleet plugin, should we tackle this as an other issue?

@jfsiii
Copy link
Contributor Author

jfsiii commented Mar 25, 2021

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

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

cc @jfsiii

@jfsiii jfsiii requested a review from Zacqary March 26, 2021 13:11
@jfsiii jfsiii added the auto-backport Deprecated - use backport:version if exact versions are needed label Mar 26, 2021
@jfsiii jfsiii enabled auto-merge (squash) March 26, 2021 13:13
@jfsiii
Copy link
Contributor Author

jfsiii commented Mar 26, 2021

@Zacqary Thanks for the review. I agree with you that those error messages should have translations. However, as @nchaulet notes, there are many places which don't yet do that. Are you OK with doing those changes in another PR? I was going to add them in this PR just to start the process, but I have another PR I'm focusing on right now and I'd really like to merge this one.

@Zacqary
Copy link
Contributor

Zacqary commented Mar 26, 2021

@jfsiii Sure, we can merge this one, but let's try to avoid adding new string literals without translations in the future.

@jfsiii jfsiii merged commit ab33df8 into elastic:master Mar 26, 2021
kibanamachine added a commit to kibanamachine/kibana that referenced this pull request Mar 26, 2021
## Summary
`/agents/bulk_upgrade` should return a response with a result for each agent given; including invalid or missing ids. It currently returns an empty object.

This PR includes commits from open PR elastic#95024. The additions from this PR are https://github.com/jfsiii/kibana/compare/bulk-reassign-response-should-include-all-given-agents..871ebcb

[TS type diff for response](https://github.com/jfsiii/kibana/compare/bulk-reassign-response-should-include-all-given-agents..871ebcb#diff-7006a6c170a608c8c7211fc218c0a6f4bc8ff642c170ea264db4b1b5545fb728)

```diff
- // eslint-disable-next-line @typescript-eslint/no-empty-interface
- export interface PostBulkAgentUpgradeResponse {}

+ export type PostBulkAgentUpgradeResponse = Record<
+   Agent['id'],
+   {
+     success: boolean;
+     error?: string;
+   }
+ >;
```

### Checklist

- [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios

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

💚 Backport successful

7.x / #95547

This backport PR will be merged automatically after passing CI.

kibanamachine added a commit that referenced this pull request Mar 26, 2021
## Summary
`/agents/bulk_upgrade` should return a response with a result for each agent given; including invalid or missing ids. It currently returns an empty object.

This PR includes commits from open PR #95024. The additions from this PR are https://github.com/jfsiii/kibana/compare/bulk-reassign-response-should-include-all-given-agents..871ebcb

[TS type diff for response](https://github.com/jfsiii/kibana/compare/bulk-reassign-response-should-include-all-given-agents..871ebcb#diff-7006a6c170a608c8c7211fc218c0a6f4bc8ff642c170ea264db4b1b5545fb728)

```diff
- // eslint-disable-next-line @typescript-eslint/no-empty-interface
- export interface PostBulkAgentUpgradeResponse {}

+ export type PostBulkAgentUpgradeResponse = Record<
+   Agent['id'],
+   {
+     success: boolean;
+     error?: string;
+   }
+ >;
```

### Checklist

- [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

Co-authored-by: John Schulz <john.schulz@elastic.co>
@jfsiii jfsiii deleted the bulk-upgrade-api-response-change branch April 6, 2021 17:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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.13.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants