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

feat(push): allow pushing composed components to registry #2996

Merged
merged 1 commit into from
Feb 20, 2025

Conversation

fibonacci1729
Copy link
Contributor

@fibonacci1729 fibonacci1729 commented Jan 27, 2025

WIP for now to flesh out the concept.

Implements #2988

Still working to add a test but the meat-and-potatoes is ready to serve.

@fibonacci1729 fibonacci1729 force-pushed the push branch 3 times, most recently from ee371d1 to 9176ca1 Compare January 30, 2025 18:10
@fibonacci1729 fibonacci1729 marked this pull request as ready for review January 30, 2025 18:10
@fibonacci1729
Copy link
Contributor Author

cc/ @itowlson a simple test is now included.

@fibonacci1729
Copy link
Contributor Author

@itowlson friendly ping that this is ready for another review!

Copy link
Collaborator

@itowlson itowlson left a comment

Choose a reason for hiding this comment

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

First off, sorry for taking so long to get to this.

I don't really feel capable of reviewing this beyond "I affirm that Brian is not sneaking in a Bitcoin miner," but from what I can tell it seems sound, and I assume you've tested it against suitable hosts which is more reassuring than any reading of mine could be!

I did have some comments on the style and the testing - they're not blockers, but I'd like to see if they're easy to address before we merge. (And please ignore the melodramatic last comment - we've already covered that but I'm struggling to reconcile myself to the outcome!)

Copy link
Collaborator

@itowlson itowlson 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 those changes - this feels much more readable now (at least to me!). Appreciate your patience with my niggling. If you're still comfortable with it then good to go! :shipit:

@fibonacci1729
Copy link
Contributor Author

Thanks for the review! I think i'll try to get 1 or 2 more 👀 just to spread awareness.

@fibonacci1729 fibonacci1729 requested a review from a team February 12, 2025 21:46
@fibonacci1729 fibonacci1729 force-pushed the push branch 2 times, most recently from 2492a32 to d69826c Compare February 19, 2025 18:35
@fibonacci1729
Copy link
Contributor Author

Ran make test locally and everything seems green; Also fixed a few lints that popped up in tests/

@fibonacci1729 fibonacci1729 merged commit be47837 into spinframework:main Feb 20, 2025
16 of 17 checks passed
@fibonacci1729 fibonacci1729 deleted the push branch February 20, 2025 21:43
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.

Feature Request: Add flag to registry push that pushes composed components
3 participants