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

Check for a nil lastTime before use #283

Merged
merged 1 commit into from
Jan 11, 2022
Merged

Check for a nil lastTime before use #283

merged 1 commit into from
Jan 11, 2022

Conversation

Nalum
Copy link
Member

@Nalum Nalum commented Dec 17, 2021

Fix: #246

Avoiding a nil-deref by checking the lastTime variable before using it.

@Nalum
Copy link
Member Author

Nalum commented Dec 17, 2021

Not too sure about how to test this, happy to get some pointers.

Copy link
Member

@pjbgf pjbgf left a comment

Choose a reason for hiding this comment

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

Not too sure about how to test this, happy to get some pointers.

I can't see an easy way to test it, apart from extracting the if-else block of code into its own function and creating a unit test just for it.

controllers/imageupdateautomation_controller.go Outdated Show resolved Hide resolved
controllers/imageupdateautomation_controller.go Outdated Show resolved Hide resolved
@Nalum
Copy link
Member Author

Nalum commented Dec 22, 2021

I can't see an easy way to test it, apart from extracting the if-else block of code into its own function and creating a unit test just for it.

I can look at doing this but is seems a little overkill to me.

Copy link
Member

@pjbgf pjbgf left a comment

Choose a reason for hiding this comment

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

I can look at doing this but is seems a little overkill to me.

agreed, besides the ongoing refactoring may help on that front too.

on a side point, I added a nit comment about simplifying the if logic for clarity.

controllers/imageupdateautomation_controller.go Outdated Show resolved Hide resolved
Copy link
Member

@pjbgf pjbgf left a comment

Choose a reason for hiding this comment

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

Nice one @Nalum, thank you for your contribution. 🙇

LGTM

@Nalum
Copy link
Member Author

Nalum commented Jan 5, 2022

Thanks for your help and patience @pjbgf 👍

@stefanprodan
Copy link
Member

@Nalum please squash all commits into a single one.

Signed-off-by: Luke Mallon (Nalum) <luke@mallon.ie>
Copy link
Member

@hiddeco hiddeco left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you @Nalum 🥇

@hiddeco hiddeco merged commit 7e0fa85 into fluxcd:main Jan 11, 2022
@Nalum Nalum deleted the issue-246 branch January 11, 2022 15:46
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.

Possible nil-deref in image-automation controller
4 participants