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

Issue #80: Times in the record of key usage should be at session level #87

Closed
wants to merge 9 commits into from

Conversation

mwatson2
Copy link
Contributor

@mwatson2 mwatson2 commented Sep 3, 2015

No description provided.

@mwatson2
Copy link
Contributor Author

I agree with all @ddorwin 's suggestions.

For the questions:

(1) Do you need to add the key IDs here or is that already handled by update()?

They need to be added somewhere. I'll check and address.

(2) Is "tfirst" consistent with good JSON naming practices?

I don't know. Happy to take suggestions.

@mwatson2 mwatson2 self-assigned this Oct 15, 2015
@ddorwin ddorwin modified the milestone: V1 Oct 16, 2015
@ddorwin
Copy link
Contributor

ddorwin commented Oct 21, 2015

The needs implementation label was making searches confusing, so I removed it.

</li>
</ul>
<p>
A <a def-id="message"></a> of type <a def-id="message-type-license-release"></a> containing the <a def-id="record-of-key-usage"></a> will be generated when <a def-id="remove"></a> is called until the record is acknowledged by a response passed to <a def-id="update"></a>.
Copy link
Contributor

Choose a reason for hiding this comment

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

We should probably look at the behavior of sessions and calling remove() after the record is acknowledged in an update() call. Nothing specific to change in this PR, but this text makes me wonder.

@mwatson2
Copy link
Contributor Author

Are we ready to merge this now ? (No green button will be pressed ...)

@ddorwin
Copy link
Contributor

ddorwin commented Oct 28, 2015

Oh, I missed that there were new commits as I only saw the comments in email. I will try to take a look tomorrow.

@ddorwin
Copy link
Contributor

ddorwin commented Nov 3, 2015

The last two (non-merge) commits LGTM. Thanks.

mwatson2 added a commit that referenced this pull request Nov 5, 2015
@mwatson2
Copy link
Contributor Author

mwatson2 commented Nov 5, 2015

Merged as 7a55532

@mwatson2 mwatson2 closed this Nov 5, 2015
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.

None yet

2 participants