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

Global defaults for image configs #887

Merged
merged 1 commit into from Sep 16, 2023
Merged

Global defaults for image configs #887

merged 1 commit into from Sep 16, 2023

Conversation

IamTheFij
Copy link
Contributor

Allows setting of image configs at a global level to act as default values.

This required a change in the model.Image struct due to a bool field not having a third, unset state. The remedy is to unmarshal into a temporary data structure to detect the presents of a field value and then use that to determine if the default value should be used.

Fixes #491

@IamTheFij IamTheFij requested a review from crazy-max as a code owner June 7, 2023 17:48
crazy-max
crazy-max requested changes Jun 11, 2023
View reviewed changes
Copy link
Owner

@crazy-max crazy-max left a comment

Choose a reason for hiding this comment

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

Sorry for the delay and thanks your contrib! Please see my review.

go.mod Outdated Show resolved Hide resolved
internal/model/image.go Outdated Show resolved Hide resolved
internal/provider/file/file_test.go Outdated Show resolved Hide resolved
internal/provider/file/file_test.go Outdated Show resolved Hide resolved
internal/provider/file/file_test.go Outdated Show resolved Hide resolved
internal/provider/file/image.go Outdated Show resolved Hide resolved
internal/provider/nomad/task_test.go Outdated Show resolved Hide resolved
@IamTheFij IamTheFij marked this pull request as draft June 12, 2023 16:18
@IamTheFij
Copy link
Contributor Author

I marked as draft so I could identify the outstanding issue, bit it appears to be ratelimitting on the e2e tests.

@IamTheFij IamTheFij marked this pull request as ready for review June 12, 2023 16:24
@IamTheFij
Copy link
Contributor Author

I read through the workflow file to recreate these tests locally and I get the same rate limit issue on master. So I'm pretty sure this isn't related to my change. What do you think, @crazy-max?

@crazy-max
Copy link
Owner

I read through the workflow file to recreate these tests locally and I get the same rate limit issue on master. So I'm pretty sure this isn't related to my change. What do you think, @crazy-max?

I think this is linked to your changes. Tests in this PR #902 run fine.

crazy-max
crazy-max requested changes Jul 2, 2023
View reviewed changes
Copy link
Owner

@crazy-max crazy-max left a comment

Choose a reason for hiding this comment

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

Seems linked to the change of watchRepo to true instead of false as it was before.

internal/app/job.go Outdated Show resolved Hide resolved
@IamTheFij
Copy link
Contributor Author

Ah! Thanks so much. I must have missed up my find and replace. I bet the reason I reproduced it on master was because I had already run the test on my branch causing me to run up my API requests prior. I'll go through and double check my defaults aren't changing.

@IamTheFij
Copy link
Contributor Author

Squashed and rebased onto master.

@crazy-max
Copy link
Owner

Looks like codecov is not happy but this is linked to new TestValidateImage that doesn't cover other attributes: https://app.codecov.io/gh/crazy-max/diun/commit/1960b18054d5f0288fd527196ba8fb6785ece25e/blob/internal/provider/common.go

Fine to look at this in follow-up.

I will review your PR asap but thanks in advance for your work on this!

@IamTheFij
Copy link
Contributor Author

So, this is not my first time being confused by codecov. I'm not sure why my patch gets dinged for this when that file had no coverage before and I added a few lines and every one of those lines in that file are covered.

@crazy-max
Copy link
Owner

crazy-max commented Jul 3, 2023 •
edited

I'm not sure why my patch gets dinged for this when that file had no coverage before and I added a few lines and every one of those lines in that file are covered.

That's because adding common_test.go will now start checking coverage of the whole common.go file so there will be an overall less coverage if this file is not fully covered in the tests. The -3.87% is for the project but 68.00% is the target coverage of your PR.

@IamTheFij
Copy link
Contributor Author

Are there any more tests you'd like me to write to get coverage up, or is this ok since I added partial coverage to an uncovered file.

IamTheFij
IamTheFij commented Aug 8, 2023
View reviewed changes
internal/provider/common.go Outdated Show resolved Hide resolved
@IamTheFij
Copy link
Contributor Author

Decided to make merging easier by adding more test cases to get common.go up to 100%.

@IamTheFij
Copy link
Contributor Author

@crazy-max Since you last reviewed this, I've improved coverage and added merging of Metadata values, which was absent last time. I found it by getting test coverage to 100%, so yay for tests!

crazy-max
crazy-max reviewed Sep 10, 2023
View reviewed changes
Copy link
Owner

@crazy-max crazy-max left a comment

Choose a reason for hiding this comment

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

Sorry forgot I got pending reviews 😥

I will take a further look next week, sorry for the delay (again!)

internal/provider/common.go Show resolved Hide resolved
internal/provider/common.go Show resolved Hide resolved
internal/provider/common.go Outdated Show resolved Hide resolved
internal/provider/common.go Show resolved Hide resolved
internal/provider/common.go Outdated Show resolved Hide resolved
internal/provider/common.go Outdated Show resolved Hide resolved
internal/provider/common.go Show resolved Hide resolved
internal/provider/nomad/task.go Outdated Show resolved Hide resolved
@IamTheFij IamTheFij mentioned this pull request Sep 11, 2023
Merged
@IamTheFij
Global defaults for image configs
60ddac4 
Allows setting of image configs at a global level to act as default
values.

This required a change in the model.Image struct due to a bool field not
having a third, unset state. The remedy is to unmarshal into a temporary
data structure to detect the presents of a field value and then use that
to determine if the default value should be used.

Fixes crazy-max#491
@IamTheFij
Copy link
Contributor Author

Rebased on master to avoid merge conflicts.

crazy-max
crazy-max requested changes Sep 15, 2023
View reviewed changes
Copy link
Owner

@crazy-max crazy-max left a comment

Choose a reason for hiding this comment

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

Just docs update missing

@crazy-max
Copy link
Owner

@IamTheFij I'm merging your PR but can you open a follow-up for docs please? Thanks.

@crazy-max crazy-max merged commit c78d877 into crazy-max:master Sep 16, 2023
31 checks passed
@crazy-max crazy-max mentioned this pull request Sep 16, 2023
Merged
@crazy-max crazy-max mentioned this pull request Sep 23, 2023
Merged
@gustav-b gustav-b mentioned this pull request Sep 24, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@crazy-max crazy-max crazy-max approved these changes

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

Successfully merging this pull request may close these issues.

Option to set watch_repo globally
2 participants
@IamTheFij @crazy-max