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

Pre-update lifecycle hook #793

Merged
merged 15 commits into from
Jun 23, 2021
Merged

Pre-update lifecycle hook #793

merged 15 commits into from
Jun 23, 2021

Conversation

yrien30
Copy link
Contributor

@yrien30 yrien30 commented Jan 21, 2021

Will close #649

@yrien30 yrien30 requested a review from simskij as a code owner January 21, 2021 09:39
@yrien30 yrien30 marked this pull request as draft January 21, 2021 10:11
@codecov
Copy link

codecov bot commented Jan 21, 2021

Codecov Report

Merging #793 (f8c9e8d) into main (6b155a1) will increase coverage by 0.91%.
The diff coverage is 52.94%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #793      +/-   ##
==========================================
+ Coverage   54.47%   55.38%   +0.91%     
==========================================
  Files          25       25              
  Lines        1452     1466      +14     
==========================================
+ Hits          791      812      +21     
+ Misses        594      590       -4     
+ Partials       67       64       -3     
Impacted Files Coverage Δ
pkg/container/client.go 14.46% <0.00%> (-0.13%) ⬇️
pkg/container/container.go 46.80% <0.00%> (-0.51%) ⬇️
internal/actions/update.go 63.79% <81.81%> (+13.31%) ⬆️

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 91bbe74...f8c9e8d. Read the comment docs.

@kevinlul
Copy link

kevinlul commented Feb 4, 2021

How can I help out? 👀 Just testing?

@yrien30
Copy link
Contributor Author

yrien30 commented Feb 9, 2021

How can I help out? 👀 Just testing?

Great if you could test it and maybe come whit suggestion on improving the documentation. I'm also not sure if there should be a own flag for skipping the lifecycle hooks when the container is stoped or restarting.

@yrien30
Copy link
Contributor Author

yrien30 commented Mar 6, 2021

I have uploaded it to ghcr.io. So If anyone would test it then you will find it here Its build for arm64.

@yrien30
Copy link
Contributor Author

yrien30 commented Mar 18, 2021

I have now tested that this works.

Exit code 1:
image
Exit code 75:
image
Container that needs update and is not running. Previously it would not get updated because the preupdate script can only run when the container is running..
image
Exit code 0:
image

@yrien30 yrien30 marked this pull request as ready for review March 18, 2021 09:12
@yrien30
Copy link
Contributor Author

yrien30 commented Apr 6, 2021

Making this a draft again since I found out that the container is not created after it had been deleted. I need figure out why..

image

@yrien30 yrien30 marked this pull request as draft April 6, 2021 11:55
@yrien30
Copy link
Contributor Author

yrien30 commented Apr 6, 2021

Tested that the container gets started again after latest fix:
image

@yrien30 yrien30 marked this pull request as ready for review April 6, 2021 18:03
@simskij
Copy link
Member

simskij commented Apr 22, 2021

Awesome! Thanks for your contribution! I can't review it right now, but will get back to it as soon as possible.

Copy link
Member

@piksel piksel left a comment

Choose a reason for hiding this comment

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

This does look good in general, but there really ought to be tests for the functionality.

Also, some run go fmt to fix things like spaces between arguments.

pkg/container/client.go Outdated Show resolved Hide resolved
pkg/container/client.go Outdated Show resolved Hide resolved
@yrien30
Copy link
Contributor Author

yrien30 commented May 1, 2021

This does look good in general, but there really ought to be tests for the functionality.

Also, some run go fmt to fix things like spaces between arguments.

I have added some test and run go fmt. Have also retested everything like I did above manualy after I merged the changes form main.

@yrien30 yrien30 requested a review from piksel May 3, 2021 10:00
@yrien30
Copy link
Contributor Author

yrien30 commented Jun 15, 2021

Any chance to get an update on whether this could be merged or need more work?

@subdavis
Copy link

Without tests, this is a 100 LoC change that several people clearly care about. Thanks to @yrien30 for putting this PR together.

As a show of good faith, I've started sponsoring https://github.com/sponsors/simskij

If this could be reviewed, merged, and released sometime soon, that would be greatly appreciated.

@simskij
Copy link
Member

simskij commented Jun 23, 2021

LGTM! Thanks for the contribution! Let's try this in :latest-dev for a bit to make sure it's stable.

@simskij simskij merged commit 145fe6d into containrrr:main Jun 23, 2021
@simskij simskij mentioned this pull request Jan 22, 2022
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.

Make watchtower skip update if pre-update lifecycle hook exits with a non-zero exit code
5 participants