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

refactor: DBTP-1766 - version provider #761

Open
wants to merge 21 commits into
base: main
Choose a base branch
from

Conversation

ksugden
Copy link
Contributor

@ksugden ksugden commented Feb 7, 2025

Addresses DBTP-


Checklist:

Title:

  • Scope included as per conventional commits
  • Ticket reference included (unless it's a quick out of ticket thing)

Description:

  • Link to ticket included (unless it's a quick out of ticket thing)
  • Includes tests (or an explanation for why it doesn't)
  • If the work includes user interface changes, before and after screenshots included in description
  • Includes any applicable changes to the documentation in this code base
  • Includes link(s) to any applicable changes to the documentation in the DBT Platform Documentation (can be to a pull request)

Tasks:

  • Run the end to end tests for this branch and confirm that they are passing

@chiaramapellimt chiaramapellimt marked this pull request as ready for review February 7, 2025 09:46
@chiaramapellimt chiaramapellimt requested a review from a team as a code owner February 7, 2025 09:46
@ksugden ksugden changed the title refactor: DBTPD-1766 - version provider refactor: DBTP-1766 - version provider Feb 7, 2025
Copy link
Contributor

@WillGibson WillGibson left a comment

Choose a reason for hiding this comment

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

Some questions.

pass


# TODO add timeouts and exception handling for requests
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this todo not address yet on purpose or by omission?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

on purpose as we were not currently handling this so need to do some investigation to decide what we should return in the case of not being able to resolve one of the versions. Will be addressed in a follow up PR as part of the same ticket.

Comment on lines +8 to +9
class VersionProvider(ABC):
pass
Copy link
Contributor

Choose a reason for hiding this comment

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

I started writing this suggestion, then realised that the two concrete implementations methods have different arguments. So I'm wondering what the benefit of the abstract class is?

Suggested change
class VersionProvider(ABC):
pass
class VersionProvider(ABC):
@abstractmethod
def get_latest_version(repo_name: str, tags: bool = False) -> SemanticVersion:
pass

Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder whether the interface can be the same if GithubVersionProvider. get_latest_version() returns whichever version is highest from the releases or tags?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We'll address this in one of the next PRs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants