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

Update version output and logic #5029

Merged
merged 9 commits into from
Apr 12, 2022
Merged

Update version output and logic #5029

merged 9 commits into from
Apr 12, 2022

Conversation

stu-k
Copy link
Contributor

@stu-k stu-k commented Apr 11, 2022

resolves #4724

Description

Most notably, updated the logic for displaying plugin information as follows:

  • If the plugin has neither the same major nor the same minor as the installed core version, it is considered incompatible
  • Then, if the latest version of the plugin can not be found, a "no version available" message will be displayed
  • Then, the plugin is compared with its latest version to display an appropriate message based on if it is ahead, matches, is behind the latest version

The tests capture the intended code paths, please review them for more information about outputs.

Checklist

  • I have signed the CLA
  • I have run this code in development and it appears to resolve the stated issue
  • This PR includes tests, or tests are not required/relevant for this PR
  • I have added information about my change to be included in the CHANGELOG.

@stu-k stu-k requested a review from a team April 11, 2022 20:46
@stu-k stu-k requested a review from a team as a code owner April 11, 2022 20:46
@cla-bot cla-bot bot added the cla:yes label Apr 11, 2022
@github-actions
Copy link
Contributor

Thank you for your pull request! We could not find a changelog entry for this change. For details on how to document a change, see the contributing guide.

Copy link
Contributor

@jtcohen6 jtcohen6 left a comment

Choose a reason for hiding this comment

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

Nice work @stu-k, this represents a meaningful improvement and refactor of the code! I left a handful of comments. Only one is a true blocker (Major.Minor compatibility comparison). The rest are smaller + cosmetic only.

There's one bigger question about the right way to present upgrade info when it's relevant to core + many plugins. I think it would be worth a more generic message that prints https://docs.getdbt.com/dbt-cli/install/overview once, rather than once for each.

core/dbt/version.py Outdated Show resolved Hide resolved
core/dbt/version.py Outdated Show resolved Hide resolved
core/dbt/version.py Outdated Show resolved Hide resolved
core/dbt/version.py Outdated Show resolved Hide resolved
core/dbt/version.py Outdated Show resolved Hide resolved
core/dbt/version.py Outdated Show resolved Hide resolved
core/dbt/version.py Outdated Show resolved Hide resolved
Copy link
Contributor

@ChenyuLInx ChenyuLInx left a comment

Choose a reason for hiding this comment

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

This looks great! Very solid refactor of the code! Left one nitpick that's only my personal preference. Can also leave it out if you feel like the current way is easier to read!

@ChenyuLInx
Copy link
Contributor

Looks like there's a mypy error, otherwise looks good!

@stu-k
Copy link
Contributor Author

stu-k commented Apr 12, 2022

@ChenyuLInx That error doesn't have to do with anything I added or changed.
The same line exists on main https://github.com/dbt-labs/dbt-core/blob/main/core/dbt/config/project.py#L194

EDIT: Whoops totally caused by my changes.

@stu-k stu-k requested a review from a team April 12, 2022 18:02
@ChenyuLInx
Copy link
Contributor

@ChenyuLInx That error doesn't have to do with anything I added or changed.
The same line exists on main https://github.com/dbt-labs/dbt-core/blob/main/core/dbt/config/project.py#L194

Yes, looks like this line is not even touched. I am not sure why mypy complains about it either. But when I run mypy on main it doesn't have any error. We will have to fix it or ignore this to get the workflow pass

@stu-k stu-k merged commit f23a403 into main Apr 12, 2022
@stu-k stu-k deleted the stu/ct-229-version-logic-update branch April 12, 2022 19:36
VersusFacit pushed a commit that referenced this pull request Apr 14, 2022
Update version output and logic
agoblet pushed a commit to BigDataRepublic/dbt-core that referenced this pull request May 20, 2022
Update version output and logic
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[CT-229] Adapter compatible message rule update
3 participants