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

ENH gets latest image if more than one #472

Merged
merged 7 commits into from
Nov 24, 2020

Conversation

maikia
Copy link
Contributor

@maikia maikia commented Nov 24, 2020

use case: when pipeline is used each next version of the image will have the name image_name_{date_time_of_creation}

this PR enables a worker to find all the images which include the name image_name in their name and to select the newest one.

This only works if the image_name is given in the config file. If the id of the image will be given the worker will always use the same image with that ID.

@maikia
Copy link
Contributor Author

maikia commented Nov 24, 2020

not sure how to make tests for it though....

@agramfort
Copy link
Collaborator

merge if you need to move on. We will know very quickly if it works :)

@codecov
Copy link

codecov bot commented Nov 24, 2020

Codecov Report

Merging #472 (1e28da3) into master (a332e0c) will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #472      +/-   ##
==========================================
+ Coverage   93.54%   93.56%   +0.01%     
==========================================
  Files          99       99              
  Lines        8496     8496              
==========================================
+ Hits         7948     7949       +1     
+ Misses        548      547       -1     
Impacted Files Coverage Δ
ramp-engine/ramp_engine/tests/test_aws.py 82.63% <100.00%> (ø)
ramp-engine/ramp_engine/base.py 93.58% <0.00%> (+1.28%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a332e0c...1e28da3. Read the comment docs.

Copy link
Collaborator

@tomMoral tomMoral left a comment

Choose a reason for hiding this comment

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

Some small nitpicks but otherwise, looks good to me.

ramp-engine/ramp_engine/aws/api.py Outdated Show resolved Hide resolved
ramp-engine/ramp_engine/aws/api.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@tomMoral tomMoral left a comment

Choose a reason for hiding this comment

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

Two nitpicks but good to go otherwise :)

ramp-engine/ramp_engine/aws/api.py Outdated Show resolved Hide resolved
ramp-engine/ramp_engine/aws/api.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@tomMoral tomMoral left a comment

Choose a reason for hiding this comment

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

LGTM!

@maikia maikia merged commit a2f585f into paris-saclay-cds:master Nov 24, 2020
@maikia
Copy link
Contributor Author

maikia commented Nov 24, 2020

Thanks @tomMoral and @agramfort

@maikia maikia deleted the launch_instance_update branch November 24, 2020 15:34
maikia added a commit to maikia/ramp-board that referenced this pull request Dec 3, 2020
* enable getting the newest image if there are more than one with the same string contained in the name

* cleanup

* cleanup

* clean up

* typo

* update the tests
glemaitre pushed a commit that referenced this pull request Dec 3, 2020
* enable getting the newest image if there are more than one with the same string contained in the name

* cleanup

* cleanup

* clean up

* typo

* update the tests
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