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

feat: add a label take precedence argument #1754

Merged
merged 8 commits into from
Sep 16, 2023

Conversation

jebabin
Copy link
Contributor

@jebabin jebabin commented Sep 7, 2023

This PR add an argument to have labels take precedence over arguments.
By default watchtower have argument taking precedence over label which is not very convenient because arguments are global while label specific to container.
This is useful to allow specific container to be updated when monitor only is set to true
it close #1745
Test has been added to ensure that container are updated when monitor only is global set to true but set to false on the container level.
Documentation has been updated to include this new argument.
Additionally documentation has been updated to better describe the label enable argument which not only disable update but also disable monitoring

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Congratulations on opening your first pull request! We'll get back to you as soon as possible. In the meantime, please make sure you've updated the documentation to reflect your changes and have added test automation as needed. Thanks! 🙏🏼

Fix dual blank line in arguments.md
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.

Thanks for helping out with this!

internal/actions/update.go Outdated Show resolved Hide resolved
docs/arguments.md Outdated Show resolved Hide resolved
@jebabin jebabin requested a review from simskij as a code owner September 8, 2023 16:53
@jebabin jebabin requested a review from piksel September 8, 2023 17:01
@codecov
Copy link

codecov bot commented Sep 10, 2023

Codecov Report

Patch coverage: 88.88% and project coverage change: +0.43% 🎉

Comparison is base (36391b0) 67.60% compared to head (2d1d8e7) 68.03%.
Report is 7 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1754      +/-   ##
==========================================
+ Coverage   67.60%   68.03%   +0.43%     
==========================================
  Files          26       26              
  Lines        2380     2387       +7     
==========================================
+ Hits         1609     1624      +15     
+ Misses        672      664       -8     
  Partials       99       99              
Files Changed Coverage Δ
pkg/container/client.go 49.47% <0.00%> (ø)
pkg/container/container.go 55.72% <87.50%> (+3.17%) ⬆️
internal/actions/update.go 70.21% <100.00%> (-0.16%) ⬇️
internal/flags/flags.go 85.37% <100.00%> (+0.16%) ⬆️
pkg/container/metadata.go 68.00% <100.00%> (+10.10%) ⬆️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@piksel
Copy link
Member

piksel commented Sep 10, 2023

I made a generalised helper to implement the logic, to make it easier to visually understand the logic/flow:

func (c Container) getContainerOrGlobalBool(globalVal bool, label string, contPrecedence bool) bool {
	if contVal, err := c.getBoolLabelValue(label); err != nil {
		if !errors.Is(err, errorLabelNotFound) {
			logrus.WithField("error", err).WithField("label", label).Warn("Failed to parse label value")
		}
		return globalVal
	} else {
		if contPrecedence {
			return contVal
		} else {
			return contVal || globalVal
		}
	}
}

Are you OK with me pushing this? I can also add some more tests to increase the coverage.

@jebabin
Copy link
Contributor Author

jebabin commented Sep 10, 2023

Of course I'm, this is clearly better

@piksel piksel changed the title ft: Add a label take precedence argument feat: add a label take precedence argument Sep 16, 2023
@piksel piksel merged commit 650acde into containrrr:main Sep 16, 2023
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.

Update only specific containers
2 participants