Skip to content

golangci-lint: migrate configuration to v2 schema #510

Merged
openshift-merge-bot[bot] merged 2 commits intocontainers:mainfrom
vyasgun:pr/golangci-v2
May 22, 2025
Merged

golangci-lint: migrate configuration to v2 schema #510
openshift-merge-bot[bot] merged 2 commits intocontainers:mainfrom
vyasgun:pr/golangci-v2

Conversation

@vyasgun
Copy link
Copy Markdown
Member

@vyasgun vyasgun commented May 12, 2025

Migrate configuration to golangci-lint v2 schema

@vyasgun vyasgun force-pushed the pr/golangci-v2 branch 4 times, most recently from 8c57e59 to f42d4ee Compare May 12, 2025 12:02
@vyasgun vyasgun self-assigned this May 14, 2025
@vyasgun vyasgun moved this to Work In Progress in Project planning: crc May 14, 2025
@vyasgun vyasgun force-pushed the pr/golangci-v2 branch 6 times, most recently from eb0be84 to bf6d445 Compare May 16, 2025 11:03
@vyasgun vyasgun moved this from Work In Progress to Ready for review in Project planning: crc May 19, 2025
Copy link
Copy Markdown
Collaborator

@cfergeau cfergeau left a comment

Choose a reason for hiding this comment

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

Overall looks good, some parts can likely be improved, either to avoid "ignore" annotations, or to avoid some checks by refactoring the code.

log.Warnf("size exceeds max limit. Resetting to: %d", math.MaxInt32)
size = math.MaxUint32
}
binary.BigEndian.PutUint32(buf, uint32(size)) //#nosec: G115. Safely checked
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

slightly worried about adding too much code in Write, txBuf, … as this could have a performance impact (but I haven’t measured it, it’s likely to not be impactful).
Haven’t given much thought to this part of the PR, so I don’t know if we can handle this differently.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

We could change the signature of Write method to Write(buf []byte, size uint32). This would mean we would only need to check in hyperkitProtocol.Write for uint16

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I suspect these lines

size := conn.protocolImpl.(streamProtocol).Buf()
conn.protocolImpl.(streamProtocol).Write(size, len(buf))
would then need a check so that we can cast to uint32 before calling into the function?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

yes, that's right. We can perform conversion here and then call the Write function. We can return an error in case of an invalid value. However, there's still hyperkitProtocol which needs uint16. We could also skip Write in case of an invalid size (but I think this is not correct):

        if size > math.MaxUint32 {
		log.Errorf("size exceeds max limit.")
	}  else {
	        binary.BigEndian.PutUint32(buf, uint32(size))
        }

@vyasgun vyasgun force-pushed the pr/golangci-v2 branch 3 times, most recently from ff7795f to 1799cfb Compare May 21, 2025 07:45
@vyasgun vyasgun requested a review from cfergeau May 21, 2025 07:55
@vyasgun
Copy link
Copy Markdown
Member Author

vyasgun commented May 21, 2025

/retest

Copy link
Copy Markdown
Collaborator

@cfergeau cfergeau left a comment

Choose a reason for hiding this comment

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

2 minor comments left :)

@cfergeau cfergeau mentioned this pull request May 21, 2025
vyasgun added 2 commits May 22, 2025 09:41
Signed-off-by: Gunjan Vyas <vyasgun20@gmail.com>
Signed-off-by: Gunjan Vyas <vyasgun20@gmail.com>
Copy link
Copy Markdown
Collaborator

@cfergeau cfergeau left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve
/hold
(until the last round of CI clears)

return err
}
atomic.AddUint64(&e.Sent, uint64(pkt.Size()))
atomic.AddUint64(&e.Sent, uint64(size)) //#nosec:G115. Safely checked
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This nosec annotation is not needed on my system (I pushed this change to your branch)

return nil, errors.Wrap(err, "cannot create tap endpoint")
}
networkSwitch := tap.NewSwitch(configuration.Debug, configuration.MTU)
networkSwitch := tap.NewSwitch(configuration.Debug, mtu)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Interestingly, tap.Switch no longer seem to be using the mtu, all use is now in the LinkEndpoint. This could be cleaned up in a followup PR.

@openshift-ci openshift-ci bot added the lgtm label May 22, 2025
@openshift-ci
Copy link
Copy Markdown
Contributor

openshift-ci bot commented May 22, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cfergeau, vyasgun

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@cfergeau
Copy link
Copy Markdown
Collaborator

/unhold

@cfergeau
Copy link
Copy Markdown
Collaborator

/unhold

@openshift-merge-bot openshift-merge-bot bot merged commit 7db13d1 into containers:main May 22, 2025
17 of 18 checks passed
@github-project-automation github-project-automation bot moved this from Ready for review to Done in Project planning: crc May 22, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants