-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
implementation for an artifactory service. #9666
Conversation
username: 'jerry', | ||
password: 'seinfeld', | ||
}, | ||
staticPreview: Artifactory.render('2.1.0'), |
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.
No matter what I did I couldn't get any of this to show up in the local "integ test server". I assume I'm doing something wrong but filled out all the relevant links/options/docs/etc anyway if only for completeness sake.
const defaultHeaders = { Accept: '*/*' } | ||
|
||
// dump query params to debug console | ||
console.debug(`***** artifactory found the following query properties ***** |
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 know if you all care about keeping this in but it was generally useful for me when debugging to see what was coming through as values for the query parameters. Let me know if you want me to rip it out.
@calebcartwright thoughts one way or another? I can continue with writing some integration tests (unless you have units which would be easier) but wanted to get your thoughts/opinions one way or another or just those from the community at large. |
One significant issue with this is that, afaik, there's no default public instance of Artifactory that we can rely on for our integration and testing. As such, this is a badge that seems like it would only be applicable for self-hosted instances and which would have no utility on the primary Shields.io instance. This is something the Shields team has historically objected to doing, so we'd need some confirmation from other members of the team that they'd be okay with breaking precedent here before considering moving forward |
As far as the implementation goes, I feel like it's going in a vastly different direction than our typical services and I'm not entirely sure why that would need to be the case. I'd suggest reviewing some of our material, linked below for reference, to reconsider the implementation or to at least help inform a subsequent explanation as to why this would require taking such a different path: |
Couple of things:
One thing this does make me think is: This isn't super common, but also the idea of a badge that is only applicable to self-hosting users is something that has come up before. I wonder if there is a way we could provide a plugin hook to allow self-hosting users to conveniently inject a custom service like this without having to maintain a fork. 🤔 Hmmmm.. |
@chris48s gets is ;) |
I'm personally aligned on these points as well. I don't think we'll want to move forward with this badge for the time being, so will go ahead and close the PR.
For reference, #9787 tracks this. Anyhow, thanks for your interest in the Shields.io project, @cdancy! |
Looking for feedback on this impl and whether or not I'm going down the correct path and if this is something you all would be interested in taking in. I more/less just copied a lot of what I saw others doing in other services and did the same here to get things up and going quick. Tests still need to be written but the initial impl works as expected with no issues to speak of.
The service itself is basically just a wrapper around Artifactory's latest version endpoint the documentation of which you can FIND HERE.
@calebcartwright let me know what you think and if this is something you all would consider moving forward on. If not, and we had to maintain this out-of-tree, is there a good process for doing so other than the obvious you all would recommend?