-
Notifications
You must be signed in to change notification settings - Fork 214
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
Recommend combination of create and apply commands instead of generate for docs #2302
Recommend combination of create and apply commands instead of generate for docs #2302
Conversation
83ec582
to
20e25ea
Compare
Will definitely need lots of reviews, looking forward to it @carolynvs 😄 |
Thanks for submitting the PR, @joshuabezaleel ! |
@VinozzZ Sure, thank you lots Yingrong! Looking forward to it 😄 |
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.
Thank you again for improving our doc! I left some comments and suggestions. Let me know what you think
mage/docs/docs.go
Outdated
@@ -74,7 +74,7 @@ func DocsPreview() { | |||
time.Sleep(time.Second) | |||
} | |||
|
|||
must.Run("open", "http://localhost:1313/docs/") | |||
// must.Run("open", "http://localhost:1313/docs/") |
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.
Is this intended change?
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.
Ah no, this wasn't intentional, commented this since the open
command didn't work on my machine :/ Thanks for pointing this out!
253bdb0
to
321fc31
Compare
Thank you so much for the really thorough and helpful review, @VinozzZ ! Should be ready for another review 😄 |
…e for docs Signed-off-by: joshuabezaleel <[email protected]>
Signed-off-by: joshuabezaleel <[email protected]>
The contributing guide currently tells people to make changes against main which is old info. Once we release v1, we'll need to update this again to stay current. Signed-off-by: Carolyn Van Slyck <[email protected]> Signed-off-by: joshuabezaleel <[email protected]>
* Fix support for insecure registries Two of our dependencies, cnab-to-oci and cnab-go, had implementations of registry communication. cnab-go for working with archives, and cnab-to-oci for pushing bundles to registries and copying bundles. Each had inconsistent or missing logic around when to skip TLS certificate validation or use plain http. So while Porter was passing the --insecure-registry flag to our dependencies, it wasn't having the intended effect. I have updated both dependencies to support unsecured (plain http) registries on loopback addresses (127.x.x.x) by default, and to skip TLS verification as requested by Porter with the --insecure-registry flag. Signed-off-by: Carolyn Van Slyck <[email protected]> * Only try to close the test registry once Signed-off-by: Carolyn Van Slyck <[email protected]> * Skip TestCLI until we change how we log errors Originally we had a test, TestCLI, that made sure that only main was logging the error, not cobra. Now that we are logging errors in the function where they occurred, we are seeing the error message print multiple times, each time that we use span.Error(). Since this isn't related to this change, I am skipping the test for now and opening an issue to decide who is responsible for writing an error to the console (main vs the originating function). Signed-off-by: Carolyn Van Slyck <[email protected]> * Run mage with -v to see which tests are slow Signed-off-by: Carolyn Van Slyck <[email protected]> * Review feedback * Add prerequisites section to the insecure registry doc * Remove TestRegistryOptions.Port * Add helper function for making a http transport with skip TLS set Signed-off-by: Carolyn Van Slyck <[email protected]> Signed-off-by: Carolyn Van Slyck <[email protected]> Signed-off-by: joshuabezaleel <[email protected]>
Signed-off-by: Carolyn Van Slyck <[email protected]> Signed-off-by: joshuabezaleel <[email protected]>
Signed-off-by: joshuabezaleel <[email protected]>
Signed-off-by: joshuabezaleel <[email protected]>
704883f
to
a406933
Compare
/azp run porter-integration |
Commenter does not have sufficient privileges for PR 2302 in repo getporter/porter |
@VinozzZ The test seems to fail on Windows smoke test 🤔 Do we have to rerun the integration test? |
@joshuabezaleel Sorry about the delay! There have been intermittent errors with our smoke and integration tests that I am working to correct. Would you please try merging the latest changes from release/v1, resolving the merge conflict, and then updating your pull request? I'll keep an eye on your build and identify if you are being impacted by this same bug I'm fixing. |
@carolynvs No worries at all! Thank you for the kind update. I think the smoke test still fails (panic because of timed out(?)) and the integration test fails at |
Yay! Tests are passing again. |
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.
Sorry for the delayed response, thank you for working with me through this. I think it's ready to go!
@carolynvs @VinozzZ Ah, so sorry for the late reply and that I missed this. Thank you for making the pipeline and test works and for merging this as well, Carolyn and Yingrong! 😃 |
What does this change
The docs currently still recommend user to use
porter parameters generate
orporter credentials generate
as a way to create Parameter Set and Credential Set for their bundle.On v1 onwards we want to change it to recommend with the combination of
porter parameters/credentials create
andporter parameters/credentials apply
for users to use instead of the interactivegenerate
one since it might be harder to document or copy/paste.What issue does it fix
Closes #1080
Notes for the reviewer
Put any questions or notes for the reviewer here.
Checklist
Reviewer Checklist