Skip to content

Conversation

@ckotamra
Copy link
Contributor

@ckotamra ckotamra commented Dec 18, 2024

TODO TDX comments were removed and corresponding issue items have been created. Closes #523

@ckotamra ckotamra requested a review from a team as a code owner December 18, 2024 23:55
@mattkur
Copy link
Contributor

mattkur commented Dec 19, 2024

Hi - Could you elaborate? Do you mean that these TODOs are all fully resolved, or just that there are now issues tracking each of the TODOs?

@ckotamra
Copy link
Contributor Author

There are issue items created to track all these TODO TDX changes. Some of them have already been resolved.

Copy link
Collaborator

@mebersol mebersol left a comment

Choose a reason for hiding this comment

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

Just removing the TODOs removes the context that goes with them.

Instead, please remove only comments that are resolved or no longer need to be tracked. For TODOs that are now tracked with issues, the code could be updated to point to the issue to easily correlate ownership/fix.

@cperezvargas
Copy link
Contributor

@ckotamra all issues originally identified by you are in this repo (I went through them this AM), to expand, rather than delete each TODO comment, please change each TODO comment to either
A) resolved is {link to closed issue}
or
B) TODO originally identified here but now tracked in {link to open issue}

@smalis-msft
Copy link
Contributor

Personally I still lean towards keeping the TODOs in the code, even if issues have been made for them, until the issue is resolved.

smalis-msft
smalis-msft previously approved these changes Jan 17, 2025
@benhillis
Copy link
Member

@ckotamra - looks like this PR is stale. Are you planning on rebasing this?

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.

[TDX] Remove TODO TDX comments from OpenVMM

6 participants