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

fix: prevent empty version folders from being created #67

Merged
merged 1 commit into from
Nov 11, 2021

Conversation

gablaxian
Copy link
Contributor

The current method to creating the version spec files involved looping over all identified versions and then over the stabilities and creating folders for all, regardless of whether a spec file was necessary.

While not a problem, per se, it conflicted with the mapping of those versions within Registry when looping over the compiled versions and checking for their existence. In that respect, if a folder is found with no file, then that should be a legitimate error.

The solution, simply, is to only create folders for which spec files exist.

@gablaxian gablaxian requested a review from a team as a code owner November 10, 2021 17:18
@gablaxian gablaxian self-assigned this Nov 10, 2021
@CLAassistant
Copy link

CLAassistant commented Nov 10, 2021

CLA assistant check
All committers have signed the CLA.

Copy link
Contributor

@cmars cmars left a comment

Choose a reason for hiding this comment

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

Thanks for this! LGTM, just a couple of small suggestions.

internal/compiler/compiler.go Show resolved Hide resolved
internal/compiler/compiler_test.go Outdated Show resolved Hide resolved
@@ -457,7 +457,7 @@
}
},
"x-snyk-api-resource": "hello-world",
"x-snyk-api-version": "2021-06-01"
"x-snyk-api-version": "2021-06-01~experimental"
Copy link
Contributor

Choose a reason for hiding this comment

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

This is somewhat puzzling to me as I'd expect this to be 2021-06-04~experimental

Copy link
Contributor

Choose a reason for hiding this comment

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

/examples/hello-world/{id} in this version comes from 2021-06-01~experimental. This version exists because of the projects addition in 2021-06-04~experimental.

Copy link
Contributor

Choose a reason for hiding this comment

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

Below you'll find that the projects path has extensions reflecting its introduction:

      "x-snyk-api-resource": "projects",
      "x-snyk-api-version": "2021-06-04~experimental"

So with this change, there will be at least one path at the version of the compiled spec. Other paths in the file may be older and/or of greater stability -- it's a snapshot of "what versions are effective at this version".

@genslein
Copy link
Contributor

cc @Todai88 I believe this would address https://github.com/snyk/v3-go-libs/blob/main/oas_router/oas_router.go#L244-L249 correct my if I'm mistaken @gablaxian @cmars ?

@gablaxian
Copy link
Contributor Author

gablaxian commented Nov 11, 2021

cc @Todai88 I believe this would address https://github.com/snyk/v3-go-libs/blob/main/oas_router/oas_router.go#L244-L249 correct my if I'm mistaken @gablaxian @cmars ?

I'm not totally sure. What this PR is fixing, is this error which you see on registry startup:

{"name":"Error","message":"ENOENT: no such file or directory, open '/home/circleci/registry/dist/web/routes/api/sarif/versions/2021-08-13~beta/spec.json'"

which occurs because folders for all stabilities are created per version, regardless of whether the endpoint exists at that stability level. So when you first start creating endoints, you are going to end up with a lot of redundant folders. And when we scan to get all the spec files, we error because we expect a file in every folder.

I think the issue you linked to is something else and, if I am not mistaken, is intended?

@gablaxian gablaxian force-pushed the fix/compile-redundant-folders branch 2 times, most recently from 5ebba63 to 3e54824 Compare November 11, 2021 12:59
@genslein
Copy link
Contributor

I think the issue you linked to is something else and, if I am not mistaken, is intended?
Yes, it was a different issue being confusing generating many folders and files more than the pre-compiled specs define. ignore me 🏃

The current method to creating the version spec files involved looping over all identified versions and then over the stabilities and creating folders for all, regardless of whether a spec file was necessary.

While not a problem, per se, it conflicted with the mapping of those versions within Registry when looping over the compiled versions and checking for their existence. In that respect, if a folder is found with no file, then that should be a legitimate error.

The solution, simply, is to only create folders for which spec files exist.
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