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

New digests #424

Merged
merged 6 commits into from
Jul 21, 2024
Merged

New digests #424

merged 6 commits into from
Jul 21, 2024

Conversation

J08nY
Copy link
Member

@J08nY J08nY commented Jul 19, 2024

Fixes #420.

@J08nY J08nY added the bug Something isn't working label Jul 19, 2024
Copy link

codecov bot commented Jul 19, 2024

Codecov Report

Attention: Patch coverage is 92.70833% with 7 lines in your changes missing coverage. Please review.

Project coverage is 66.83%. Comparing base (c5b4f3d) to head (ee63131).
Report is 6 commits behind head on main.

Files Patch % Lines
src/sec_certs/sample/cc_scheme.py 91.08% 5 Missing ⚠️
src/sec_certs/sample/cc.py 91.67% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #424      +/-   ##
==========================================
- Coverage   68.36%   66.83%   -1.53%     
==========================================
  Files          62       62              
  Lines        7579     7553      -26     
==========================================
- Hits         5181     5047     -134     
- Misses       2398     2506     +108     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@J08nY
Copy link
Member Author

J08nY commented Jul 19, 2024

@adamjanovsky What do you think about this solution? It moves to what we discussed in #420, using the filename parts of the st and report links. It also keeps the old digest attributes, perhaps a better naming there would be possible. I was thinking of having "alt_dgsts" or "old_dgsts" property that would have the older digests and would be a set. What do you think? This is necessary to be able to map the old to the new on the site (as well as for other of our users for some kind of compat).

@J08nY J08nY requested a review from adamjanovsky July 19, 2024 15:48
@J08nY J08nY self-assigned this Jul 19, 2024
@J08nY J08nY added the cc Related to CC certification label Jul 19, 2024
J08nY added 2 commits July 19, 2024 18:50
@J08nY J08nY merged commit da10d50 into main Jul 21, 2024
6 checks passed
@J08nY J08nY deleted the fix/new-dgsts branch July 21, 2024 14:18
@adamjanovsky
Copy link
Collaborator

Well, late to the review I guess :)

Anyways, I agreed with the proposal, didn't go through the implementation in the detail. One think to keep in mind is our usage of digests in Notebooks. I suppose that the backward-compatability redirect only applies to web, right?

What's going to happen when you load an old dataset with a new tool version?

@J08nY
Copy link
Member Author

J08nY commented Jul 22, 2024

Anyways, I agreed with the proposal, didn't go through the implementation in the detail. One think to keep in mind is our usage of digests in Notebooks. I suppose that the backward-compatability redirect only applies to web, right?

Yes right now. However, you can get the old digests from the certs.

What's going to happen when you load an old dataset with a new tool version?

Interesting thought, haven't considered it much as I was looking at it from the POV of the web where a fresh run is running. However, I tested this a bit and what happens is that the new digests take precedence. So after loading it you get a dataset with new digests only (because dgst is a property computed from attributes) and if you store it again you will get the new digests also in the JSON. Basically, the old digests are ignored.

Also, regarding the quick merge. I wanted to get this in quickly because of the Monday update run. The web has been broken for a while and I wanted to fix it. This was a pre-requisite.

@adamjanovsky
Copy link
Collaborator

Interesting thought, haven't considered it much as I was looking at it from the POV of the web where a fresh run is running. However, I tested this a bit and what happens is that the new digests take precedence. So after loading it you get a dataset with new digests only (because dgst is a property computed from attributes) and if you store it again you will get the new digests also in the JSON. Basically, the old digests are ignored.

Cool, that's what I anticipated.

Also, regarding the quick merge. I wanted to get this in quickly because of the Monday update run. The web has been broken for a while and I wanted to fix it. This was a pre-requisite.

Understood 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working cc Related to CC certification
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CCportal URL change
2 participants