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

Fixing failed metav1.Time test error for verify #315

Closed
wants to merge 2 commits into from

Conversation

ChaosInTheCRD
Copy link
Collaborator

When running make test I noticed that they were failing due to an error caused by the Expires field not accepting time.Time variables. I don't have full context but changing it to the below fixed the tests. Worth getting eyes from someone responsible for these tests in the past for sanity 😅.

@colek42
Copy link
Member

colek42 commented Nov 29, 2023

cc @jkjell I think this was introduced when you make the changes for the data provider.

Copy link
Member

@jkjell jkjell left a comment

Choose a reason for hiding this comment

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

Yeah, the change looks good to me. 👍 This gives us consistent json serialization with time (and it's an input for the generated code from the external data provider).

I think once you run a go mod tidy it should fix up the errors from the CI and I'll approve.

Signed-off-by: chaosinthecrd <[email protected]>
@mikhailswift
Copy link
Member

Strange part is I seem to remember fixing this at one point 🤔 . Guess I either imagined that or never got it into main.

@ChaosInTheCRD
Copy link
Collaborator Author

Ah. I see why this is a problem.

I am replacing go-witness in my go.mod with my local module. This was causing the error above with tests. Of course, this repository follows version 1.17 of go-witness, which still has this field as type time.Time.

This will need to be fixed eventually, but not yet.

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.

4 participants