-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Fix feature server docker image tag generation in pr integration tests #2077
Fix feature server docker image tag generation in pr integration tests #2077
Conversation
Signed-off-by: Tsotne Tabidze <tsotne@tecton.ai>
Codecov Report
@@ Coverage Diff @@
## master #2077 +/- ##
===========================================
- Coverage 83.31% 58.18% -25.13%
===========================================
Files 100 100
Lines 8084 8084
===========================================
- Hits 6735 4704 -2031
- Misses 1349 3380 +2031
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
Signed-off-by: Tsotne Tabidze <tsotne@tecton.ai>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: adchia, tsotnet The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
@@ -37,3 +37,6 @@ | |||
|
|||
# Default FTS port | |||
DEFAULT_FEATURE_TRANSFORMATION_SERVER_PORT = 6569 | |||
|
|||
# Environment variable for feature server docker image tag | |||
DOCKER_IMAGE_TAG_ENV_NAME: str = "FEAST_SERVER_DOCKER_IMAGE_TAG" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't DOCKER_IMAGE_TAG_ENV_NAME
be something "feature server" specific?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why would we use different tags for different servers? Also, we could always rename this if the necessity arises. It won't break anything.
Signed-off-by: Tsotne Tabidze tsotne@tecton.ai
What this PR does / why we need it: Sometimes is can happen that
build-docker-image
andintegration-test-python
workflows run on different git commit hashes. This is because the HEAD commit hash is a merge commit of master and current PR HEAD. If the master gets updated between these 2 workflows, the HEAD commit hash changes, causing integration tests to try to use a wrong feature server docker image tag.Instead, we'll be setting the commit hash as a workflow output in
build-docker-image
, then pass it tointegration-test-python
as an environment variable and use that in integration tests. This will guarantee that the same tag is being used.Which issue(s) this PR fixes:
Fixes #
Does this PR introduce a user-facing change?: