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

feat(TCOMP-2475): Migration from higher version not protected for a connector #898

Merged
merged 19 commits into from
Jun 27, 2024
Merged

feat(TCOMP-2475): Migration from higher version not protected for a connector #898

merged 19 commits into from
Jun 27, 2024

Conversation

undx
Copy link
Member

@undx undx commented Jun 6, 2024 •
edited by acatoire
Loading

@sonar-eks sonar-eks

This comment has been minimized.

Sign in to view
@sonar-eks sonar-eks

This comment has been minimized.

Sign in to view
@sonar-eks sonar-eks

This comment has been minimized.

Sign in to view
@sonar-eks sonar-eks

This comment has been minimized.

Sign in to view
@sonar-eks sonar-eks

This comment has been minimized.

Sign in to view
@sonar-eks sonar-eks

This comment has been minimized.

Sign in to view
@sonar-eks sonar-eks

This comment has been minimized.

Sign in to view
@undx undx changed the title feat(TCOMP-2475): default undefined version to 1 feat(TCOMP-2475): Migration from higher version not protected for a connector Jun 19, 2024
@sonar-eks sonar-eks

This comment has been minimized.

Sign in to view
2 similar comments
@sonar-eks sonar-eks

This comment has been minimized.

Sign in to view
@sonar-eks sonar-eks

This comment has been minimized.

Sign in to view
ozhelezniak-talend
ozhelezniak-talend reviewed Jun 20, 2024
View reviewed changes
...ponent-server/src/main/java/org/talend/sdk/component/server/front/ComponentResourceImpl.java
@@ -566,9 +566,15 @@ public Map<String, String> migrate(final String id, final int version, final Map
.status(Status.NOT_FOUND)
.entity(new ErrorPayload(COMPONENT_MISSING, "Didn't find component " + id))
.build()));
if (version > comp.getVersion() || version == comp.getVersion()) {
if (version > comp.getVersion()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not >=?
If the config version == the version, we shouldn't migrate it, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we call migration if versions ==

Copy link
Member Author

Choose a reason for hiding this comment

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

There's a comment on jira issue concerning studio's migration handling.
Migration fails in that case and the job cannot be opened.
Same behavior observed in slack topic before the revert.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh my...
Okay

ozhelezniak-talend
ozhelezniak-talend reviewed Jun 20, 2024
View reviewed changes
Copy link
Contributor

@ozhelezniak-talend ozhelezniak-talend left a comment

Choose a reason for hiding this comment

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

I have a question about the call of migration when the versions are equal.
And also I don't see a protection when the incoming version is higher than the one that is registered in component-runtime.
And I thought one of the points is to have some notification about it. Warning at least.
Maybe I'm missing a point.

@undx
Copy link
Member Author

undx commented Jun 20, 2024 •
edited
Loading

I have a question about the call of migration when the versions are equal. And also I don't see a protection when the incoming version is higher than the one that is registered in component-runtime. And I thought one of the points is to have some notification about it. Warning at least. Maybe I'm missing a point.

Answered to equals in comment.
There's a protection and warning preventing component migration call from higher version here
https://github.com/Talend/component-runtime/pull/898/files/4c91f86d3b6a5fdadda74bc4bb3a49450bbd7b98#diff-c897c2077dde44ed22410fa091a71724ed24211cfde6dd20348e11bf49a4d30aR570-R572

@ozhelezniak-talend
Copy link
Contributor

I have a question about the call of migration when the versions are equal. And also I don't see a protection when the incoming version is higher than the one that is registered in component-runtime. And I thought one of the points is to have some notification about it. Warning at least. Maybe I'm missing a point.

Answered to equals in comment. There's a protection and warning preventing component migration call from higher version here https://github.com/Talend/component-runtime/pull/898/files/4c91f86d3b6a5fdadda74bc4bb3a49450bbd7b98#diff-c897c2077dde44ed22410fa091a71724ed24211cfde6dd20348e11bf49a4d30aR570-R571

So it will a warning...
Oh, don't know about that
It's really looks like smth is fishy is going on when someone tries to run the configuration that is higher than is known (registered in) by component-runtime

@undx
Copy link
Member Author

undx commented Jun 20, 2024 •
edited
Loading

I have a question about the call of migration when the versions are equal. And also I don't see a protection when the incoming version is higher than the one that is registered in component-runtime. And I thought one of the points is to have some notification about it. Warning at least. Maybe I'm missing a point.

Answered to equals in comment. There's a protection and warning preventing component migration call from higher version here https://github.com/Talend/component-runtime/pull/898/files/4c91f86d3b6a5fdadda74bc4bb3a49450bbd7b98#diff-c897c2077dde44ed22410fa091a71724ed24211cfde6dd20348e11bf49a4d30aR570-R571

So it will a warning... Oh, don't know about that It's really looks like smth is fishy is going on when someone tries to run the configuration that is higher than is known (registered in) by component-runtime

Warning and call to migrate is skipped. Agree also.

ozhelezniak-talend
ozhelezniak-talend previously approved these changes Jun 20, 2024
View reviewed changes
Copy link
Contributor

@ozhelezniak-talend ozhelezniak-talend left a comment

Choose a reason for hiding this comment

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

LGTM

@acatoire acatoire force-pushed the undx/TCOMP-2475-migration-prot branch from 81a1961 to 316b039 Compare June 26, 2024 14:50
@sonar-eks sonar-eks

This comment has been minimized.

Sign in to view
2 similar comments
@sonar-eks sonar-eks

This comment has been minimized.

Sign in to view
@sonar-eks sonar-eks
Copy link

sonar-eks bot commented Jun 26, 2024

Passed

Analysis Details

0 Issues

  • Bug 0 Bugs
  • Vulnerability 0 Vulnerabilities
  • Code Smell 0 Code Smells

Coverage and Duplications

  • Coverage 100.00% Coverage (54.30% Estimated after merge)
  • Duplications 0.00% Duplicated Code (1.30% Estimated after merge)

Project ID: org.talend.sdk.component:component-runtime

View in SonarQube

ozhelezniak-talend
ozhelezniak-talend approved these changes Jun 27, 2024
View reviewed changes
Copy link
Contributor

@ozhelezniak-talend ozhelezniak-talend left a comment

Choose a reason for hiding this comment

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

lgtm

@undx undx merged commit 9f3aaf7 into master Jun 27, 2024
6 checks passed
@undx undx deleted the undx/TCOMP-2475-migration-prot branch June 27, 2024 09:47
undx added a commit that referenced this pull request Jul 4, 2024
@undx
feat(TCOMP-2475): Migration from higher version not protected for a c…
21dff94 
…onnector (#898)

(cherry picked from commit 9f3aaf7)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ozhelezniak-talend ozhelezniak-talend ozhelezniak-talend approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@undx @ozhelezniak-talend @acatoire