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.

Sign up for GitHub

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

Jump to bottom

Continuous deployment Python script #80

Merged
merged 47 commits into from
Jan 2, 2024
Merged

Continuous deployment Python script #80

merged 47 commits into from
Jan 2, 2024

Conversation

mira-miracoli
Copy link
Contributor

@mira-miracoli mira-miracoli commented Dec 14, 2023 •
edited
Loading

Script that

  • builds the image with packer
  • converts the image with qemu-img convert
  • (optional) uploads the image to OpenStack
  • (optional) copies the image to sn06 static/vgcn directory

@kysrpex kysrpex linked an issue Dec 15, 2023 that may be closed by this pull request
Release management for VGCN images #78
Closed
6 tasks
@mira-miracoli mira-miracoli marked this pull request as ready for review December 15, 2023 14:59
bgruening
bgruening reviewed Dec 16, 2023
View reviewed changes
build.py Outdated Show resolved Hide resolved
build.py Outdated Show resolved Hide resolved
build.py Outdated Show resolved Hide resolved
build.py Outdated Show resolved Hide resolved
build.py Outdated Show resolved Hide resolved
kysrpex
kysrpex reviewed Dec 19, 2023
View reviewed changes
Copy link
Contributor

@kysrpex kysrpex left a comment •
edited
Loading

Choose a reason for hiding this comment

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

This is an extremely thorough review, but I wanted to invest the time given that this script is critical. We have seen the effect of neglected image builds numerous times (e.g. BRAKER3, probably cgroups, and many other examples from the past).

To illustrate the importance of the script, I want to draw an analogy between it and the technical revision of ambulances, if the screws on the wheels are loose, they fall away and you can't even send the doctor when somebody is dying (or you send him/her walking and other people die at the hospital meanwhile). That's exactly what is happening to us right now with numerous bugs.

Please look at the comments from a distance, do not get lost in them. Despite the fact that you see a comment every few lines, I am commenting on minor details or cases where the spec does not match what we agreed. Overall this is script is solving the problem.

.gitignore Outdated Show resolved Hide resolved
ansible/internal.yml Outdated Show resolved Hide resolved
build.py Outdated Show resolved Hide resolved
build.py Outdated Show resolved Hide resolved
build.py Outdated Show resolved Hide resolved
build.py Outdated Show resolved Hide resolved
build.py Outdated Show resolved Hide resolved
build.py Show resolved Hide resolved
environment.yml Show resolved Hide resolved
build.py Outdated Show resolved Hide resolved
mira-miracoli and others added 7 commits December 19, 2023 17:45
@mira-miracoli @kysrpex
remove unnecessary linebreaks
d953c66 
Co-authored-by: José Manuel Domínguez <[email protected]>
@mira-miracoli @kysrpex
Update .gitignore
b898deb 
Co-authored-by: José Manuel Domínguez <[email protected]>
@mira-miracoli
make parser as function
2d3a60c
@mira-miracoli @kysrpex
add license to spinner class
d6107de 
Co-authored-by: José Manuel Domínguez <[email protected]>
@mira-miracoli @kysrpex
remove unnecessary linebreaks
3c5ce69 
Co-authored-by: José Manuel Domínguez <[email protected]>
@mira-miracoli @kysrpex
remove unnecessary linebreaks
2227162 
Co-authored-by: José Manuel Domínguez <[email protected]>
@mira-miracoli @kysrpex
Update doc
6b97b23 
Co-authored-by: José Manuel Domínguez <[email protected]>
mira-miracoli and others added 16 commits December 19, 2023 18:24
@mira-miracoli @kysrpex
no append
1616455 
Co-authored-by: José Manuel Domínguez <[email protected]>
@mira-miracoli @kysrpex
remove choices
07cd13a 
Co-authored-by: José Manuel Domínguez <[email protected]>
@mira-miracoli
wrap argparse in function
c071715
@mira-miracoli @kysrpex
pathlib.path constants
1cedd6d 
Co-authored-by: José Manuel Domínguez <[email protected]>
@mira-miracoli @kysrpex
remove anaconda from docs
39be50e 
Co-authored-by: José Manuel Domínguez <[email protected]>
@mira-miracoli @kysrpex
pathlib objects look better
45468b4 
Co-authored-by: José Manuel Domínguez <[email protected]>
@mira-miracoli @kysrpex
another simple path join
0392ffc 
Co-authored-by: José Manuel Domínguez <[email protected]>
@mira-miracoli @kysrpex
and one more
505ef7c 
Co-authored-by: José Manuel Domínguez <[email protected]>
@mira-miracoli @kysrpex
that should be a variable
36f041c 
Co-authored-by: José Manuel Domínguez <[email protected]>
@mira-miracoli @kysrpex
fix order
9c8c903 
Co-authored-by: José Manuel Domínguez <[email protected]>
@mira-miracoli @kysrpex
more paths to join
60ee6d6 
Co-authored-by: José Manuel Domínguez <[email protected]>
@mira-miracoli @kysrpex
use pathlib join
3b6a4b0 
Co-authored-by: José Manuel Domínguez <[email protected]>
@mira-miracoli @kysrpex
copy list
8b363fa 
Co-authored-by: José Manuel Domínguez <[email protected]>
@mira-miracoli @kysrpex
add architecture
1f153c5 
Co-authored-by: José Manuel Domínguez <[email protected]>
@mira-miracoli @kysrpex
reorder assemble name
e1c6d93 
Co-authored-by: José Manuel Domínguez <[email protected]>
@mira-miracoli @kysrpex
use pathlibs exist method
ef2e646 
Co-authored-by: José Manuel Domínguez <[email protected]>
@mira-miracoli
Copy link
Contributor Author

Thank you for the thorough review, I think I learned a lot :)

@kysrpex kysrpex changed the title Python script Continuous deployment Python script Dec 20, 2023
kysrpex and others added 2 commits December 20, 2023 16:29
@kysrpex
Compute commit time in assemble_timestamp
56b9e2f 
Compute commit time in `assemble_timestamp` instead of build time as agreed in usegalaxy-eu#78.
@kysrpex
Workaround for including double quotes in PKR_VAR_groups env variable
72db2e4 
Prevents "SyntaxError: f-string expression part cannot include a backslash".

```
In [1]: f"[{','.join('\"' + x + '\"' for x in self.provisioning)}]"
  Cell In[1], line 1
    f"[{','.join('\"' + x + '\"' for x in self.provisioning)}]"
                                                               ^
SyntaxError: f-string expression part cannot include a backslash
```
@kysrpex
Copy link
Contributor

kysrpex commented Dec 20, 2023 •
edited
Loading

@mira-miracoli Consider this approved, I just want to run it before hitting approve.

A good reason to run it may be https://github.com/usegalaxy-eu/issues/issues/505 if it is caused by something that needs to be fixed in the images. I'll look into the issue and if the images need to be fixed, I'll build and upload them using this, then hit approve.

@kysrpex kysrpex mentioned this pull request Dec 20, 2023
Closed
6 tasks
@mira-miracoli
Copy link
Contributor Author

was it intentional that 8b363fa leaves the generic tag in the name?
I'm fine with that, I just wanted to note that because, I think in #78 the idea was to only mention !generic if it is missing
But I am fine with both ways

@mira-miracoli mira-miracoli mentioned this pull request Dec 21, 2023
Draft
@bgruening bgruening merged commit 6f78a5f into usegalaxy-eu:dev Jan 2, 2024
@bgruening
Copy link
Member

Thanks, everyone! And a happy new year!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kysrpex kysrpex kysrpex left review comments

@bgruening bgruening bgruening left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Release management for VGCN images
3 participants
@mira-miracoli @kysrpex @bgruening