-
Notifications
You must be signed in to change notification settings - Fork 25
Adding new processor class for artifactory #932
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
Conversation
isc-dchui
left a comment
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.
@isc-jlechtne Left a few comments/thoughts!
isc-eneil
left a comment
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 my first round of comments @isc-jlechtne. Looks pretty good to me!
|
One more note @isc-jlechtne: this could use some integration tests (not sure how you'd obscure Arti credentials so may be a bit tricky) |
isc-kiyer
left a comment
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.
@isc-jlechtne This is a good draft of migrating over stuff from AngularArtifact but its not yet generic for any IPM consumer (need to get out of the lens of just our use case). We can chat about it offline if you want but in its current state, this is not generally usable for pulling any arbitrary artifact from Artifactory.
isc-dchui
left a comment
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 a few questions
isc-kiyer
left a comment
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.
@isc-jlechtne few comments up but general logic looks good!
|
Took a look-- no comments beyond those that Keshav already provided regarding using environment variables over params. Looks good! @isc-jlechtne |
isc-kiyer
left a comment
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.
@isc-jlechtne Looks good! Last few minor comments. We need some tests for this though so any thought as to how to add those?
@isc-kiyer I talked with @isc-eneil yesterday and she volunteered to take on writing the tests to better swarm on this. How to write the tests without exposing credentials required for the tests, however, is something both of us did not have an initial answer for. |
isc-dchui
left a comment
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.
Looks good, just one more thing!
isc-kiyer
left a comment
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.
@isc-jlechtne few minor comments. Also, I think it would be good to work with @isc-dchui to figure out the tarball unpacking error when using FileBinaryTar since we should use one consistent approach for unpacking/packing tarballs
isc-dchui
left a comment
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 a few small things
isc-eneil
left a comment
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.
@isc-jlechtne Approved assuming the very small nits are addressed.
d11885f
This PR addresses #935
This is for creating a new generic resource processor to cover bundling an artifact with a package and deploying it to a final location on install.
Behavior on packaging:
Behavior on activation:
DeployDirectoryorDevDirectoryif defined and installed in dev mode.