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

types: do not require go binary to be present during configuration #63

Merged
merged 1 commit into from
Aug 8, 2022
Merged

types: do not require go binary to be present during configuration #63

merged 1 commit into from
Aug 8, 2022

Conversation

muvaf
Copy link
Member

@muvaf muvaf commented Aug 8, 2022

Description of your changes

Uptest was failing for some reason and it turns out we introduced a dependency to types package from config and types requires go binary to be present to load the packages that contain reference types. Those types are quite simple, so I just removed package loading and initialized them manually - removing the need to load any package.

I have:

  • Read and followed Crossplane's contribution process.
  • Run make reviewable to ensure this PR is ready for review.
  • Added backport release-x.y labels to auto-backport this PR if necessary.

How has this code been tested

make reviewable doesn't report any diff.

@muvaf muvaf requested a review from sergenyalcin August 8, 2022 14:00
@muvaf muvaf changed the title types: do not require go binary to be present so that configs can be … types: do not require go binary to be present during configuration Aug 8, 2022
@muvaf muvaf requested a review from ytsarev August 8, 2022 14:14

commentOptional, err = comments.New("")
if err != nil {
panic(errors.Errorf("cannot build new comment for reference fields"))
Copy link
Member

Choose a reason for hiding this comment

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

@muvaf is it safe to remove all additional error checking that is happening in init()?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, since it does what &comments.Comment does but by parsing the given text. Since we don't have text to parse, we can just initialize with &comments.Comment{} and not have an error to worry about at all.

Copy link
Member

@ytsarev ytsarev left a comment

Choose a reason for hiding this comment

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

Thanks a lot for the fix!

@muvaf muvaf merged commit 307f99f into crossplane:main Aug 8, 2022
@muvaf muvaf deleted the ref-types branch August 8, 2022 14:30
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.

2 participants