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

Bugfix.issue#478 #541

Merged
merged 0 commits into from
Aug 29, 2018
Merged

Bugfix.issue#478 #541

merged 0 commits into from
Aug 29, 2018

Conversation

vishnumohan1991
Copy link
Contributor

Renamed authorID field in oe_identity structure to signerID

@@ -68,7 +68,7 @@ typedef struct _oe_identity

/** The author ID for the enclave.
Copy link
Contributor

Choose a reason for hiding this comment

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

Change comment as well.

@@ -68,7 +68,7 @@ typedef struct _oe_identity

/** The author ID for the enclave.
* For SGX enclaves, this is the MRSIGNER value */
uint8_t author_id[OE_AUTHOR_ID_SIZE];
uint8_t signerID[OE_SIGNER_ID_SIZE];
Copy link
Contributor

Choose a reason for hiding this comment

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

@anitagov Is there any impact to existing documentation due to this field rename? I think you worked on documenting some of the attestation APIs.

Copy link
Contributor

Choose a reason for hiding this comment

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

This definitely affects the html files but .md file does not show the data structures in report.h. @vishnumohan1991 - Good idea to run make refman-source and verify that there are no modified .md files.

Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of updating the .md files, I would recommend simply removing the .md files as part of task #545 in your next PR.

Copy link
Contributor

@mikbras mikbras left a comment

Choose a reason for hiding this comment

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

Thank you Vishnu for doing this. Looks good except the name of field should be signer_id.

common/report.c Outdated
@@ -54,10 +54,10 @@ static void _oe_parse_sgx_report_body(
sizeof(reportBody->mrenclave));

OE_STATIC_ASSERT(
sizeof(parsedReport->identity.author_id) >=
sizeof(parsedReport->identity.signerID) >=
Copy link
Contributor

@mikbras mikbras Aug 15, 2018

Choose a reason for hiding this comment

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

The name of the field should be "signer_id". We can change it now or let the camel2snake tool catch it before release.

Copy link
Contributor

Choose a reason for hiding this comment

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

Please update it to identity.signer_id before merging.

Copy link
Contributor

@CodeMonkeyLeet CodeMonkeyLeet left a comment

Choose a reason for hiding this comment

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

Please fix the SignerID -> signer_id as part of this changelist.

.cspellignore Outdated
@@ -309,6 +309,7 @@ Spectre
ld
pubKey
AuthorID
Copy link
Contributor

Choose a reason for hiding this comment

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

You should be able to remove AuthorID since we don't use that term in OE anymore … similarly, see if you can remove SignerID once you switch it to match snake casing (i.e. signer_id)

@CodeMonkeyLeet CodeMonkeyLeet added this to Backlog in Public preview via automation Aug 17, 2018
@CodeMonkeyLeet CodeMonkeyLeet added the functionality Issue describes an enhancement or addition of functionality to Open Enclave SDK label Aug 17, 2018
@CodeMonkeyLeet CodeMonkeyLeet added this to the 2018.08 milestone Aug 17, 2018
@CodeMonkeyLeet CodeMonkeyLeet moved this from Backlog to In review in Public preview Aug 17, 2018
Public preview automation moved this from In review to Done Aug 20, 2018
Public preview automation moved this from Done to In progress Aug 21, 2018
@CodeMonkeyLeet CodeMonkeyLeet moved this from In progress to In review in Public preview Aug 22, 2018
@CodeMonkeyLeet
Copy link
Contributor

@vishnumohan1991 Please investigate and resolve the build failure. You might need to merge from master again and see if that resolves the issue for you.

@vishnumohan1991
Copy link
Contributor Author

The build conflicts are resolved due to merging but Jenkins test is not running against the latest pushed changes. I have verified my commits against coffelake and kabylake machines in debug,release,relwithdebinfo and no issues found yet While checking the build it shows the automated build in Jenkins environment is queued and not processing at all. Can you comment on this behavior?

@CodeMonkeyLeet
Copy link
Contributor

@vishnumohan1991 John has since restored the CI pipeline so I've merged your changes.

@CodeMonkeyLeet CodeMonkeyLeet merged commit fee0685 into master Aug 29, 2018
Public preview automation moved this from In review to Done Aug 29, 2018
@CodeMonkeyLeet CodeMonkeyLeet deleted the bugfix.issue#478 branch August 29, 2018 15:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
functionality Issue describes an enhancement or addition of functionality to Open Enclave SDK
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

5 participants