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

Add core_version to server api query #126

Merged
merged 6 commits into from Nov 11, 2020
Merged

Add core_version to server api query #126

merged 6 commits into from Nov 11, 2020

Conversation

zanieb
Copy link
Contributor

@zanieb zanieb commented Nov 9, 2020

Summary

Adds a human readable version string to the server api query indicating the Prefect Core version.

Importance

Displaying the core version the Server is using in the UI is helpful.

Gets going on #120 -- Will need UI update

Checklist

This PR:

  • adds new tests (if appropriate)
  • adds a change file in the changes/ directory (if appropriate)

Copy link
Member

@cicdw cicdw left a comment

Choose a reason for hiding this comment

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

Code wise this looks good; before we move forward with this implementation though, I want to raise a UX issue I foresee: I suspect most users use prefect server start to stand up Server. The images that this uses are always built prior to tagging prefect for release, meaning that if I'm on 0.13.4 of Prefect (for example), I'll see something like 0.13.3+63.g4c764ddb8 as my core_version which I suspect will confuse people.

I'm not 100% sure how to avoid this chicken / egg problem

@znicholasbrown
Copy link
Contributor

Could we add an env variable to the build step (something like target_release) to use for official releases but fall back to the commit hash for non-tagged spinups?

@zanieb
Copy link
Contributor Author

zanieb commented Nov 9, 2020

Code wise this looks good; before we move forward with this implementation though, I want to raise a UX issue I foresee: I suspect most users use prefect server start to stand up Server. The images that this uses are always built prior to tagging prefect for release, meaning that if I'm on 0.13.4 of Prefect (for example), I'll see something like 0.13.3+63.g4c764ddb8 as my core_version which I suspect will confuse people.

I'm not 100% sure how to avoid this chicken / egg problem

I see two options:

  • Rebuild the docker images with the correct version of Prefect Core, it seems a little weird that we are retagging when the core code could be out of date anyway
  • Pass an environment variable in the prefect server start docker-compose and use that if present e.g.
        "version": os.getenv("PREFECT_SERVER_VERSION", prefect_server.__version__),
        "core_version": os.getenv("PREFECT_CORE_VERSION", prefect.__version__),

Could we add an env variable to the build step (something like target_release) to use for official releases but fall back to the commit hash for non-tagged spinups?

The build step for the docker image? This would still be out of date like Chris mentioned.

@cicdw
Copy link
Member

cicdw commented Nov 9, 2020

I vote we take the environment variable route:

"core_version": os.getenv("PREFECT_CORE_VERSION", prefect.__version__),

Purely as FYI: the only thing that Server relies on from Prefect Core is serialization logic for state objects and some Flow objects, so more often than not we don't need to touch the Server images when we make core changes. Given that, I think this core_version field is more of a "schema compatibility" signal, so you know what version of Core "sanctioned" this particular server image.

@codecov-io
Copy link

Codecov Report

Merging #126 (47253db) into master (4c8e562) will decrease coverage by 0.03%.
The diff coverage is 100.00%.

Copy link
Member

@cicdw cicdw left a comment

Choose a reason for hiding this comment

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

really minor request to re-categorize as enhancement, otherwise this LGTM!

changes/pr126.yaml Outdated Show resolved Hide resolved
Co-authored-by: Chris White <chris@prefect.io>
@zanieb
Copy link
Contributor Author

zanieb commented Nov 11, 2020

Note this will require a PR in prefect to pass this variable and a PR in ui to display it.

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.

None yet

4 participants