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

Recommend combination of create and apply commands instead of generate for docs #2302

Merged
merged 10 commits into from
Sep 6, 2022

Conversation

joshuabezaleel
Copy link
Contributor

What does this change

The docs currently still recommend user to use porter parameters generate or porter 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 and porter parameters/credentials apply for users to use instead of the interactive generate 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

  • Did you write tests?
  • Did you write documentation?
  • Did you change porter.yaml or a storage document record? Update the corresponding schema file.
  • If this is your first pull request, please add your name to the bottom of our Contributors list. Thank you for making Porter better! 🙇‍♀️

Reviewer Checklist

  • Comment with /azp run test-porter-release if a magefile or build script was modified
  • Comment with /azp run porter-integration if it's a non-trivial PR

Sorry, something went wrong.

@joshuabezaleel
Copy link
Contributor Author

joshuabezaleel commented Aug 17, 2022

Will definitely need lots of reviews, looking forward to it @carolynvs 😄

@VinozzZ
Copy link
Contributor

VinozzZ commented Aug 17, 2022

Thanks for submitting the PR, @joshuabezaleel !
Just an FYI, Carolyn is going on vacation so I will be reviewing the PR in the meantime

@joshuabezaleel
Copy link
Contributor Author

@VinozzZ Sure, thank you lots Yingrong! Looking forward to it 😄

Copy link
Contributor

@VinozzZ VinozzZ left a 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

@@ -74,7 +74,7 @@ func DocsPreview() {
time.Sleep(time.Second)
}

must.Run("open", "http://localhost:1313/docs/")
// must.Run("open", "http://localhost:1313/docs/")
Copy link
Contributor

@VinozzZ VinozzZ Aug 18, 2022

Choose a reason for hiding this comment

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

Is this intended change?

Copy link
Contributor Author

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!

@joshuabezaleel
Copy link
Contributor Author

Thank you so much for the really thorough and helpful review, @VinozzZ ! Should be ready for another review 😄

joshuabezaleel and others added 7 commits August 26, 2022 09:52
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]>

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.
Signed-off-by: joshuabezaleel <[email protected]>
Signed-off-by: joshuabezaleel <[email protected]>
@joshuabezaleel
Copy link
Contributor Author

/azp run porter-integration

@azure-pipelines
Copy link

Commenter does not have sufficient privileges for PR 2302 in repo getporter/porter

@joshuabezaleel
Copy link
Contributor Author

@VinozzZ The test seems to fail on Windows smoke test 🤔 Do we have to rerun the integration test?

@carolynvs
Copy link
Member

@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.

@joshuabezaleel
Copy link
Contributor Author

joshuabezaleel commented Sep 6, 2022

@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 TestBindRuntimeDriverConfiguration.

image
image

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
@carolynvs
Copy link
Member

Yay! Tests are passing again.

Copy link
Contributor

@VinozzZ VinozzZ left a 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!

@VinozzZ VinozzZ merged commit 9b9cadd into getporter:release/v1 Sep 6, 2022
@joshuabezaleel
Copy link
Contributor Author

@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! 😃

@joshuabezaleel joshuabezaleel deleted the docs-v1-create-apply branch September 8, 2022 16:45
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.

Rethink interactive commands?
4 participants