-
-
Notifications
You must be signed in to change notification settings - Fork 255
feat: add new custom resolver to fetch the reference from private repo #1810
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
base: master
Are you sure you want to change the base?
Conversation
…and add new command auth add
Changeset has been generated for this PR as part of auto-changeset workflow.Please review the changeset before merging the PR.
If you are a maintainer or the author of the PR, you can change the changeset by clicking here Tip If you don't want auto-changeset to run on this PR, you can add the label |
🦋 Changeset detectedLatest commit: 25860cc The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Tests and documentation need to be updated, i will update after getting ack from this approach : )
![]()
![]() New feature:
![]() Repo URL format: https://github.com/AayushSaini101/AayushSaini101/tree/main
![]() |
src/core/parser.ts
Outdated
schema: 'https', | ||
order: 1, | ||
|
||
canRead: (uri: URI) => uri.hostname() === 'raw.githubusercontent.com', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why checking for GitHub here? 🤔 References can be in many places, not just GitHub, and even in such a case the GitHub deployment might be in a private domain, which is pretty common in medium to large companies.
src/core/parser.ts
Outdated
|
||
read: async (uri: URI) => { | ||
const url = uri.toString(); | ||
const normalized = normalizeRawGitUrl(uri); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if the URL is not pointing to a GitHub repo? This could perfectly be a Schema Registry URL, an S3 bucket, some random URL, or even a GitLab or BitBucket URL.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fmvilas, we can limit this PR for the GitHub repo only because we know proper mapping for this for example, when the user provide the URL in this format 'https://github.com/AayushSaini101/AayushSaini101/blob/main/user-signedup.yaml#/payload', we know we have to modify the URL in this format n https://api.github.com/repos/${owner}/${repo}/contents/${path}?ref=${branch}
to obtain the data from the private template, but we don't about the Gitlabm. or other BitBucket and other services modification, we can start with the github private url approach, then we can start updating bit by bit according to the requirements from the users thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mmm 🤔 this is being applied to all URLs and normalizeRawGitUrl
assumes it's a GitHub URL. I'm fine with only doing the mapping for GitHub for now but other URLs shouldn't be modified. You know what I mean?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, at this time we can consider with GithubURL only, in future we can update according the requirements, like for Gitlab and other services based on the requirements cc: @fmvilas
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to be sure we're understanding each other. If I'm understanding it correctly, right now we're assuming all URLs are GitHub URLs, which is wrong because that would make parser stop working with custom URLs. In the normalizeRawGitUrl
function we should be checking if it's actually a GitHub URL, otherwise, leave it intact. Mappings for GitLab and Bitbucket can wait but breaking all URLs except the GitHub ones is definitely not acceptable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, i need to change the functionnaming properly i will make it generic so that we can normalise other url as well in the future based on the requirements
/u |
|
@fmvilas For the private Github URL should be in this format: like https://api.github.com/repos/AayushSaini101/AayushSaini101/contents/user-signedup.yaml, not direct URL like we simple copy the path of the URL from the private repo, this is the github url, not sure about the private repo, in short we need to compare each and every url and create special check to find whether it is matching with the stored pattern or not, we can start with the github and gitlab pattern then we can slowly slowly add the comparison format as we grow thanks |
@asyncapi/bounty_team |
/u |
@AayushSaini101 I'm not sure I follow. I think you can translate from one to another, right? For instance, if I give you https://github.com/AayushSaini101/a-repo/blob/master/asyncapi.yaml (which is the URL I can copy from the browser), you can transform it to https://api.github.com/repos/AayushSaini101/a-repo/contents/asyncapi.yaml or whatever format you need it to be, right? The idea is to make it really easy for the user. Likewise, if my company has a Github installation behind the https://source.mycompany.com URL, then a URL like https://source.mycompany.com/AayushSaini101/a-repo/blob/master/asyncapi.yaml should also be possible to resolve. In short, picture this, you're working at a company and have an AsyncAPI or JSON/YAML file in a private repo. You want to reference it using $ref and it should just work out of the box. Sure, the user has to provide the necessary details to resolve these URLs but it should be fairly easy once it's configured. |
yes, i can tranform/configured the URL for the Github because i know the format for the accepted URL in the case of the Github, but for the case of another services like registry or any private registry what should be the correct URL that we need to consider for the custom resolver, that's why for this issue we can enable feature Github then in future according to the requirements we can add bit by bit @fmvilas |
/u |
@fmvilas tried to added some test cases thanks: : ) |
a3230f9
to
824bdb9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code looks fine to me. Just wanted to know the use cases for this (is it just limited to github private repo).
Also @fmvilas what should be the proposed way of testing the auth?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left comment on the tests.
IMHO, you should mock the parser functionality that's trying to make an HTTP request. When you're testing the private URL, it should fake a 401 Unauthorized HTTP response. Perhaps a 404 Not found response too, since some servers will give you that a 404 instead of a 401.
The way tests are right now, they're not testing the new feature. Let me know if you need guidance for testing.
@@ -0,0 +1,88 @@ | |||
import { expect } from 'chai'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file is definitely not needed. It's not testing any part of our code.
} | ||
}); | ||
|
||
it('should validate document with private GitHub reference', async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is testing that the document fails to validate because the $ref is in a private URL and we haven't configured how to resolve it. IMHO, the name of the test is not reflecting what this is testing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest something like the following:
it('should validate document with private GitHub reference', async () => { | |
it('should fail to validate document with private GitHub reference when not properly configured', async () => { |
const result = await validationService.validateDocument(specFile, options); | ||
|
||
// The validation succeeds but the document is invalid due to unresolved ref | ||
expect(result.success).to.equal(true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this test is passing we're doing something wrong. result.success
shouldn't be true
because a key part of the document couldn't even be resolved.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.

There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't you find it weird that success === true
but the document is invalid?
expect(result.success).to.equal(true); | ||
if (result.success) { | ||
expect(result.data).to.have.property('status'); | ||
expect(result.data?.status).to.equal('invalid'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having result.success === true
and result.data?.status === 'invalid'
is definitely weird. Don't you think so?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
More comments as before I was focusing on the tests but now I'm realizing the implementation is not there yet.
|
||
const authInfo = await ConfigService.getAuthForUrl(url); | ||
|
||
if (url.includes('github.com') && url.includes('/blob/')) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The following url would pass this check and it shouldn't: https://mycompany.com/something?url=github.com/org/repo/blob/whatever
.
You should deconstruct the URL and check for:
- host === 'github.com'
- path starts with (or contains) '/blob/'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make sense thanks : )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added thanks
throw new Error(`Failed to fetch GitHub URL: ${url} - ${res.statusText}`); | ||
} | ||
return await res.text(); | ||
} else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if the URL is not from GitHub but it's still private? That should be covered too. Actually, many big companies don't have their GitHub installation on github.com but on their own domains. Or maybe they want to use it with a schema registry like the one from Confluent.
Just to clarify, what I meant by "limiting to GitHub in this PR" is the ability to translate URLs from user-facing style to API URLs, as we're doing on line 102.
But if a URL is added to config.auth, we should attach the Authorization
header to the request.
To clarify, this is not just for GitHub private repos. The URL translation feature is just for GitHub though.
We should mock the resolver and fake the status code and response we get. We should be testing that unless we pass the |
Co-authored-by: Fran Méndez <[email protected]>
Thanks for the clarification @fmvilas I thought we just needed to add for the GitHub support only, it can extend other private URL as well:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another round of review. If I were you, I'd start with the tests. Write them first. Even if they don't pass. Write the tests as you'd like them to pass. Then adjust the logic to make the tests pass. It becomes way easier this way.
}; | ||
|
||
/** | ||
* Custom GitHub URL resolver for private repositories |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* Custom GitHub URL resolver for private repositories | |
* Custom URL resolver for private repositories |
url = createHttpWithAuthResolver().convertGitHubWebUrl(url); | ||
} | ||
|
||
// Only require authentication for GitHub URLs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment is not true. We should not require authentication only for GitHub URLs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make sense thanks
Co-authored-by: Fran Méndez <[email protected]>
Co-authored-by: Fran Méndez <[email protected]>
Co-authored-by: Fran Méndez <[email protected]>
Co-authored-by: Fran Méndez <[email protected]>
Co-authored-by: Fran Méndez <[email protected]>
Co-authored-by: Fran Méndez <[email protected]>
Co-authored-by: Fran Méndez <[email protected]>
|
Resolves: #1796