[Ingest Manager] Fix for comparing versions with -SNAPSHOT suffix #80742
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
neptunian
added
bug
neptunian
changed the title
| }); | ||
| it('returns false if agent reports upgradeable, with agent snapshot version === kibana snapshot version', () => { | ||
| expect( | ||
| isAgentUpgradeable( |
There was a problem hiding this comment.
i'm wondering whether this should be upgradable.
e.g you're running snapshot 8.0.0 but want to update to todays snapshot
There was a problem hiding this comment.
in the UI we only show upgradeable if the version is less than the kibana version, regardless of whether its a snapshot. Also the agent version should never have the '-SNAPSHOT' suffix in it (it does not currently), I just put it in case that were to change. It currently only reports the number in the metadata.
If we want to make an exception where if an agent is a snapshot that the versions can be equal and still upgradeable, I would need to check the agent local metadata to see whether its a snapshot and consider it upgradeable when versions are the same. I am fine with doing that, but not sure how common this case would be. I'd rather add it as an enhancement in the next version. I am not sure this makes much sense to have at the moment because if we are always using whether or not Kibana is on a snapshot as the version, we shouldn't be checking whether or not the agent is on a snapshot. For example, agent is a snapshot but kibana is not, we check that Agent is a snapshot and let it upgrade even though its the same version as kibana, then we provide a stable version download url because kibana is not a snapshot, and so they do not even download the snapshot anyway. Unless we want to do a check that agent and kibana are both snapshots.
| @@ -135,4 +135,35 @@ describe('Ingest Manager - isAgentUpgradeable', () => { | |||
| true | |||
| ); | |||
| }); | |||
| it('returns false if agent reports upgradeable, with agent snapshot version === kibana version', () => { | |||
| expect( | |||
| isAgentUpgradeable(getAgent({ version: '7.9.0-SNAPSHOT', upgradeable: true }), '7.9.0') | |||
There was a problem hiding this comment.
are you also checking the option where agent reports
version: 7.9.0
upgradeable: true
snapshot: true
There was a problem hiding this comment.
Do you mean checking whether or not it is a snapshot? It does not test for snapshot because it does not matter if its a snapshot whether or not its upgradeable (unless we decide to filter out snapshots). But I can add the option anyway as part of the tests. Let me know if I am misunderstanding or mistaken with the logic.
|
Pinging @elastic/ingest-management (Team:Ingest Management) |
nchaulet
approved these changes
Oct 16, 2020
| if (!agentVersionNumber) throw new Error('agent version is invalid'); | ||
| const kibanaVersionNumber = semver.coerce(kibanaVersion); | ||
| if (!kibanaVersionNumber) throw new Error('kibana version is invalid'); | ||
| return semver.lt(agentVersionNumber, kibanaVersionNumber); |
There was a problem hiding this comment.
nitpick: I think this is doing the same not sure which approach we should favorise https://github.com/elastic/kibana/blob/master/x-pack/plugins/ingest_manager/server/services/agents/checkin/state_new_actions.ts/#L127-L137
There was a problem hiding this comment.
@nchaulet The thing that makes me a bit paranoid is semver.lt('7.9.0-SNAPSHOT', '7.9.0', { includePrerelease: true }); or semver.lt('7.9.0-SNAPSHOT', '7.9.0', { includePrerelease: false }); evaluates to being true, and in our case because a version is a snapshot, it shouldn't be considered less than when the versions are the same but one is a snapshot. Is that not the case in your example?
x-pack/plugins/ingest_manager/server/routes/agent/upgrade_handler.ts
Outdated
Show resolved
Hide resolved
jfsiii
approved these changes
Oct 16, 2020
x-pack/test/ingest_manager_api_integration/apis/fleet/agents/upgrade.ts
Outdated
Show resolved
Hide resolved
💛 Build succeeded, but was flaky
Test FailuresFirefox UI Functional Tests.test/functional/apps/visualize/_tsvb_chart·ts.visualize app visual builder metric should populate fields for basic functionsStandard OutStack TraceMetrics [docs]page load bundle size
History
To update your PR or re-run it, just comment with: |
neptunian
added a commit
to neptunian/kibana
that referenced
this pull request
Oct 16, 2020
…astic#80742) * remove -SNAPSHOT from kibana version * add integration tests with -SNAPSHOT version of kibana * update isAgentUpgradeable to compare version numbers only * continue to send the kibana version with snapshot suffix to agent * cleanup code into one function * fix test to check for snapshot before adding suffix
neptunian
added a commit
to neptunian/kibana
that referenced
this pull request
Oct 16, 2020
…astic#80742) * remove -SNAPSHOT from kibana version * add integration tests with -SNAPSHOT version of kibana * update isAgentUpgradeable to compare version numbers only * continue to send the kibana version with snapshot suffix to agent * cleanup code into one function * fix test to check for snapshot before adding suffix
neptunian
added a commit
that referenced
this pull request
Oct 16, 2020
…0742) (#80910) * remove -SNAPSHOT from kibana version * add integration tests with -SNAPSHOT version of kibana * update isAgentUpgradeable to compare version numbers only * continue to send the kibana version with snapshot suffix to agent * cleanup code into one function * fix test to check for snapshot before adding suffix
neptunian
added a commit
that referenced
this pull request
Oct 16, 2020
…0742) (#80911) * remove -SNAPSHOT from kibana version * add integration tests with -SNAPSHOT version of kibana * update isAgentUpgradeable to compare version numbers only * continue to send the kibana version with snapshot suffix to agent * cleanup code into one function * fix test to check for snapshot before adding suffix
gmmorris
added a commit
to gmmorris/kibana
that referenced
this pull request
Oct 19, 2020
* master: (43 commits) [ML] Transforms: Fix tab ids for expanded row. (elastic#80666) server logs config paths to use for runner (elastic#52980) Fix audit logger logging to console even when disabled (elastic#80928) skip flaky suite (elastic#80929) Added Enterprise Search config to kibana-docker (elastic#80872) skip flaky suite (elastic#80914) [keystore_cli] parse values as JSON before adding to keystore (elastic#80848) [Ingest Manager] Fix for comparing versions with -SNAPSHOT suffix (elastic#80742) ECS audit logging (elastic#74640) [Uptime] Add client-side unit tests for remaining synthetics code (elastic#80215) [Security_Solution][Resolver] Promote z-index on node labels (elastic#80854) Move renderHeaderActions back into mount useEffect + update tests (elastic#80861) [Reporting] Document Network Policy configuration (elastic#80431) [Reporting] Add contextual documentation for CSV Max Bytes setting (elastic#80782) Add catch for Enterprise Search sending back a 401 response instead of redirect (elastic#80757) [Actions] Back Button on Add Connector Flyout (elastic#80160) removing `kibana_datatable` in favor of `datatable` (elastic#80548) [Alerting UI] Updating 'Add new' wording (elastic#80509) [Docs] Document Encrypted Saved Objects functionality. (elastic#80183) [Discover] fix auto-refresh (elastic#80635) ...
gmmorris
added a commit
to gmmorris/kibana
that referenced
this pull request
Oct 19, 2020
* master: (23 commits) [ML] Transforms: Fix tab ids for expanded row. (elastic#80666) server logs config paths to use for runner (elastic#52980) Fix audit logger logging to console even when disabled (elastic#80928) skip flaky suite (elastic#80929) Added Enterprise Search config to kibana-docker (elastic#80872) skip flaky suite (elastic#80914) [keystore_cli] parse values as JSON before adding to keystore (elastic#80848) [Ingest Manager] Fix for comparing versions with -SNAPSHOT suffix (elastic#80742) ECS audit logging (elastic#74640) [Uptime] Add client-side unit tests for remaining synthetics code (elastic#80215) [Security_Solution][Resolver] Promote z-index on node labels (elastic#80854) Move renderHeaderActions back into mount useEffect + update tests (elastic#80861) [Reporting] Document Network Policy configuration (elastic#80431) [Reporting] Add contextual documentation for CSV Max Bytes setting (elastic#80782) Add catch for Enterprise Search sending back a 401 response instead of redirect (elastic#80757) [Actions] Back Button on Add Connector Flyout (elastic#80160) removing `kibana_datatable` in favor of `datatable` (elastic#80548) [Alerting UI] Updating 'Add new' wording (elastic#80509) [Docs] Document Encrypted Saved Objects functionality. (elastic#80183) [Discover] fix auto-refresh (elastic#80635) ...
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Fleet uses the Kibana version to determine which version the agent can upgrade to. If Kibana is running as a snapshot, the version will have the
-SNAPSHOTprefix in it. When comparing versions we should not include the-SNAPSHOT.