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

Checker: Fix checking of missing files #441

Merged
merged 5 commits into from
Sep 19, 2023
Merged

Checker: Fix checking of missing files #441

merged 5 commits into from
Sep 19, 2023

Conversation

JanHoefelmeyer
Copy link
Contributor

Part of #438

Currently, any file discrepancies between the different file retrieval methods is reported in integrity.
This PR changes the checker so it reports any file missing from a list in their respective retrieval methods Requirement

Copy link
Contributor

@s-l-teichmann s-l-teichmann left a comment

Choose a reason for hiding this comment

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

LGTM

@bernhardreiter bernhardreiter changed the title Fix missing Checker: Fix checking of missing files Aug 24, 2023
@ThomasJunk
Copy link
Contributor

ThomasJunk commented Aug 24, 2023

Code looks good, BUT

I have a problem while testing.

Using the example data from the standard

"2020/example_company_-_2020-yh4711.json","2020-07-01T10:09:07Z"

results in

...
{
 "type": 2,
 "text": "Fetching https://127.0.0.1/.well-known/csaf/2020/example_company_-_2020-yh4711.json failed: Status code 404 (404 Not Found)"
},
...

Which is a result OK.

But there seems to be no complaint that this isn't found in any ROLIE or some similar message (which should be according to @JanHoefelmeyer ).

More:

Using

"2019/rhsa-2019_1862.json","2020-07-01T10:09:07Z"

results in

{
              "type": 2,
              "text": "Fetching https://127.0.0.1/.well-known/csaf/2019/rhsa-2019_1862.json failed: Status code 404 (404 Not Found)"
            },
...
            {
              "type": 2,
              "text": "https://localhost:8443/.well-known/csaf/amber/2019/rhsa-2019_1862.json in ROLIE, not in changes.csv"
            },
            {

which seems to be confusing. Going one step further:

Using

"amber/2019/rhsa-2019_1862.json","2020-07-01T10:09:07Z"

results in:

            {
              "type": 2,
              "text": "https://localhost:8443/.well-known/csaf/amber/2019/rhsa-2019_1862.json in ROLIE, not in changes.csv"
            },

which is confusing.

"amber/2019/rhsa-2019_1862.json","2020-07-01T10:09:07Z" shouldn't be standard conform.
I expected it either to be found and the errormessage about it missing wouldn't appear or it would be listed as "not found" like with the example data.

Am I testing wrong here or is there a bug hidden?

Aside: The date part seems to be neglectable as long a date is present in an entry.

@ThomasJunk
Copy link
Contributor

Okay using URLs seems to work. I misread the standard saying:

The file changes.csv MUST contain the filename

which should read

The file changes.csv MUST contain URL pointing to the file.

Copy link
Contributor

@ThomasJunk ThomasJunk left a comment

Choose a reason for hiding this comment

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

LGTM and using URLs it works.

@s-l-teichmann
Copy link
Contributor

Jan mentioned yesterday there is something wrong with this he still wants to fix. I currently do not know any more details. Postponing merge ATM.

Copy link
Member

@bernhardreiter bernhardreiter left a comment

Choose a reason for hiding this comment

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

@JanHoefelmeyer please fix what you have planned to fix.

@bernhardreiter
Copy link
Member

(I have added a review so that merging is blocked until @JanHoefelmeyer can do his fixes next week.)

@s-l-teichmann
Copy link
Contributor

(I have added a review so that merging is blocked until @JanHoefelmeyer can do his fixes next week.)

The "right" way to prevent this is to set it back to draft.

@JanHoefelmeyer
Copy link
Contributor Author

Opened #451 and #452 since the issues of the PR discussed here do not originate from these changes, nor are they part of #438.

@JanHoefelmeyer JanHoefelmeyer marked this pull request as ready for review September 4, 2023 10:45
Copy link
Contributor

@s-l-teichmann s-l-teichmann left a comment

Choose a reason for hiding this comment

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

Still LGTM

Copy link
Contributor

@ThomasJunk ThomasJunk left a comment

Choose a reason for hiding this comment

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

LGTM & Works

@JanHoefelmeyer JanHoefelmeyer dismissed bernhardreiter’s stale review September 19, 2023 13:51

No actual changes requested.

@JanHoefelmeyer JanHoefelmeyer merged commit 2a82c53 into main Sep 19, 2023
2 checks passed
@bernhardreiter bernhardreiter deleted the fix_missing branch September 19, 2023 15:53
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.

4 participants