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

vdk-heartbeat: cover requirements.txt automatic installs #2393

Merged
merged 1 commit into from
Jul 25, 2023

Conversation

antoniivanov
Copy link
Collaborator

@antoniivanov antoniivanov commented Jul 11, 2023

Upon vdk deploy the Control Service is responsible for installing all external dependecies defined in requirements.txt of the data job so they can be used during a cloud run.

A recent outage in one of our customers caused data jobs to ignore their requirements.txt and not install any external libraries. It was trace to be due to a release of data job base
image

CICD of the open source VDK did not catch this.

I am extending VDK heartbeat to test for the issue so we can catch it in the future.

See also #2391

Upon `vdk deploy` the Control Service is responsible for installing all
external dependecies defined in requirements.txt of the data job
so they can be used during a cloud run.

Recent outage in one of our customers which caused data jobs have not
had their requirements installed. It was trace to be due to a release of
[data job base
image](https://hub.docker.com/layers/versatiledatakit/data-job-base-python-3.11)

This was not caught by CICD of the open source VDK.

This change aim to address this gap and to cover this improtnat
functionality for correct installation of data job's python
requirements.
@antoniivanov antoniivanov force-pushed the person/aivanov/vdk-heartbeat branch from deff8c1 to 423d004 Compare July 11, 2023 17:08
@murphp15
Copy link
Collaborator

Is this PR just a test or a fix for the issue too?
I dont see a fix for the issue?

@antoniivanov
Copy link
Collaborator Author

antoniivanov commented Jul 14, 2023

Is this PR just a test or a fix for the issue too? I dont see a fix for the issue?

It is only a test. The fix(es) would be separate PR(s). But it fails to reproduce the issue. So I am trying to figure out why ...

It's still valuable coverage. So likely I will keep it.

@antoniivanov
Copy link
Collaborator Author

Is this PR just a test or a fix for the issue too? I dont see a fix for the issue?

The fix is in #2429

@DeltaMichael
Copy link
Contributor

It is only a test. The fix(es) would be separate PR(s). But it fails to reproduce the issue. So I am trying to figure out why ...

Wait, I'm confused now. 😄 If you're just testing something, please mark it as draft.

@antoniivanov
Copy link
Collaborator Author

It is only a test. The fix(es) would be separate PR(s). But it fails to reproduce the issue. So I am trying to figure out why ...

Wait, I'm confused now. 😄 If you're just testing something, please mark it as draft.

No, I mean , this is not fixing the issue , The fix is in separate PR (#2429).
I am extending VDK heartbeat to test for the issue so we can catch it in the future.

@antoniivanov antoniivanov merged commit ffdee8b into main Jul 25, 2023
@antoniivanov antoniivanov deleted the person/aivanov/vdk-heartbeat branch July 25, 2023 13:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants