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

Create new empty gh-pages branch if missing #163

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

manics
Copy link
Member

@manics manics commented Jun 10, 2022

This makes it easier to publish a new chart. If you think this feature is worthwhile I'll add some tests.

@consideRatio
Copy link
Member

This feature makes sense to me. The implementation seems robust enough and safe enough (safe as in no/low risk of overriding some content).

@manics manics changed the title WIP: Create new empty gh-pages branch if missing Create new empty gh-pages branch if missing Jun 19, 2022
@manics manics added the enhancement New feature or request label Jun 19, 2022
@manics manics marked this pull request as ready for review June 19, 2022 21:12
Copy link
Member

@consideRatio consideRatio left a comment

Choose a reason for hiding this comment

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

Did you mean to include the push parameter to the publish_pages function? It doesn't seem to have a purpose for this change.

I think the test seemed a bit too detailed, where the key thing to test would be that we have the new git switch --orphan gh-pages call rather than re-verify all other parts. For example, if someone like myself would change a call with ["helm", "repo", "index", "--url", "https://example.org/chartpress"] to ["helm", "repo", "index", "--url=https://example.org/chartpress"] suddenly this test named test_publish_pages_firsttime would error.

@manics
Copy link
Member Author

manics commented Jun 22, 2022

As far as I can tell all the publish_pages tests are mocked, which means we aren't fully testing it. For example the existing test_publish_pages test wouldn't succeed, since it uses test-chart as the chartname when the chart directory under tests/test_helm_chart is testchart. If you remove the mock:

$> helm package test-chart --dependency-update --destination /tmp/tmpdevp15ck/
Error: stat test-chart: no such file or directory
FAILED

I added the push arg because all the preceding commands are local, which means they can be run for real. If you think it's better I could remove the use of CheckCallWrapper (i.e. don't verify any commands at all), and instead focus on verifying the output files?

@consideRatio
Copy link
Member

👍 on adding the push parameter to publish_pages as it helps you run tests!

If you think it's better I could remove the use of CheckCallWrapper (i.e. don't verify any commands at all), and instead focus on verifying the output files?

Yes I think that would be better as this otherwise locks in how chartpress should behave in sharp detail beyond the scope of testing that we manage to handle the situation when a gh-pages branch isn't available etc as indicated by test_publish_pages_firsttime.

If you disagree I suggest we go for a merge straight away though. If we run into a test failure on this if something is changed down the line that breaks this test, we can address it then as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants