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(deploy): added a --set-default flag to set the deployed version the new default #210

Conversation

noirbizarre
Copy link

@noirbizarre noirbizarre commented Feb 29, 2024

Hi 👋🏼

Given I haven't a CONTRIBUTING file or any contribution process in the README, I allowed myself to submit a pull request.

This pull request adds a --set-default/-d flag to the deploy command, allowing to set the published version as the new default.
Documentation/README has been updated accordingly, and related tests have been added.

@jimporter
Copy link
Owner

jimporter commented Feb 29, 2024

Thanks for the patch. However, I'm not sure what the benefit of this feature is. There's already the separate mike set-default command if you need it. Could you describe how this helps?

More generally though, it's probably a bad sign if you need to call set-default more than once per project. I'd recommend setting up a latest alias (or something) and making that the default, since then the default doesn't need to change. That should also help webcrawlers (especially if you set up canonical URLs).

@noirbizarre
Copy link
Author

noirbizarre commented Feb 29, 2024

Automation is the reason: you can't set the default before having published the version you want as default. This allows to have it automated on first publish.
If not, it means either:

  • I to do it manually for each project I bootstrap after the first publication (which is totally defeating the automation purpose)
  • I have to call systematically the 2 commands, implying:
    • 2 commits for each deployment instead of one
    • potentially the 2nd one always empty

With this change, I can make it a single commit and avoid forcing an empty one for each deployment.

(I won't enter into details, but I also have some other scripting/aliases reasons which make it harder to have both command called in some cases).

@jimporter
Copy link
Owner

jimporter commented Feb 29, 2024

Automation is the reason: you can't set the default before having published the version you want as default. This allows to have it automated on first publish. If not, it means either:

  • I to do it manually for each project I bootstrap after the first publication (which is totally defeating the automation purpose)

I'm not sure I follow. Would it help if mike set-default added an --allow-undefined option to support setting a default for a version that doesn't exist yet?

  • I have to call systematically the 2 commands, implying:

    • 2 commits for each deployment instead of one
    • potentially the 2nd one always empty

mike doesn't generate empty commits unless you ask for them via --allow-empty. mike deploy $VERSION && mike set-default $VERSION does exactly the same thing as your PR with the sole exception that when $VERSION isn't the current default, there's a second commit.

If $VERSION is already the default, then mike set-default $VERSION should be a no-op. If it's not, that's a bug.

@noirbizarre
Copy link
Author

noirbizarre commented Feb 29, 2024

mike doesn't generate empty commits unless you ask for them via --allow-empty. mike deploy $VERSION && mike set-default $VERSION does exactly the same thing as your PR with the sole exception that when $VERSION isn't the current default, there's a second commit.

No it's not exactly the same, it will do 2 different commits, one for the deployment, 1 for the set-default.

If $VERSION is already the default, then mike set-default $VERSION should be a no-op. If it's not, that's a bug.

It's not exactly no-op, it is raising a warning:

warning: nothing changed in commit
  To create a commit anyway, retry with --allow-empty

It's creating support noise because people ask why there is a warning because it is the only warning appearing in the summary.
If I add --allow-empty, I end with having 2 commits each time, the second one being empty if no change.

So I would say, yes, it would help having a --allow-undefined on set-default but this seems more works and less safeness than allowing defining the default on deploy.
The alternative would be to be able to silent the warning on set-default when it is expected.

Note that I am OK with any solution, I just submitted this one as I thought it was the one providing the best compromise between additional code to maintain and provided capabilities.

@jimporter
Copy link
Owner

jimporter commented Feb 29, 2024

mike doesn't generate empty commits unless you ask for them via --allow-empty. mike deploy $VERSION && mike set-default $VERSION does exactly the same thing as your PR with the sole exception that when $VERSION isn't the current default, there's a second commit.

No it's not exactly the same, it will do 2 different commits, one for the deployment, 1 for the set-default.

Yeah, that's what the part about "with the sole exception" says. I don't see an issue with having a second commit sometimes.

It's creating support noise because people ask why there is a warning because it is the only warning appearing in the summary. If I add --allow-empty, I end with having 2 commits each time, the second one being empty if no change.

Maybe a --quiet flag would be useful to add for this, although I'm not sure it's worth it. In this case you could probably just redirect to /dev/null and not worry about it. I'll think about it though.

So I would say, yes, it would help having a --allow-undefined on set-default but this seems more works and less safeness than allowing defining the default on deploy. The alternative would be to be able to silent the warning on set-default when it is expected.

Doing it on deploy doesn't really make sense since deploy supports deploying to multiple versions (with all but the first being an alias). Since the set-default code is all optimized around setting the default to an alias (which can't be the first version named in mike deploy), it's best to have it be separate so that you can choose the correct version-or-alias to make default.

I'll add an --allow-undefined option for sure though, since that's pretty simple and fixes an unnecessary ordering constraint on setting up mike.

jimporter added a commit that referenced this pull request Feb 29, 2024
@noirbizarre
Copy link
Author

Maybe a --quiet flag would be useful to add for this, although I'm not sure it's worth it. In this case you could probably just redirect to /dev/null and not worry about it. I'll think about it though.

A --quiet flag would do it 👍🏼 (only commit when needed, do not warn when nothing require attention)
Redirecting errors and warnings to /dev/null will hide real errors when it happens (IMHO it should be a last resort solution).

Doing it on deploy doesn't really make sense since deploy supports deploying to multiple versions (with all but the first being an alias). Since the set-default code is all optimized around setting the default to an alias (which can't be the first version named in mike deploy), it's best to have it be separate so that you can choose the correct version-or-alias to make default.

That's your opinion from your own experience. You can't say what "make sense" or what'"s best" for my experience and need. I did submit this pull request after trying the above solutions and reaching the conclusion that this would be the best solution. And in-fine a change is still required to make it work so it's not total nonsense. (We seem to have different use cases and what's best to you might not be to me)

Anyway, thanks 🙏🏼 if you add an --allow-undefined and/or a --quiet flag on set-default, I'll adjust my cases to it, this will do (Initially I wanted to save you the cost of doing this change yourself with this PR).

@jimporter
Copy link
Owner

That's your opinion from your own experience. You can't say what "make sense" or what'"s best" for my experience and need.

I'm saying what makes sense for the design of this application, not anything in particular about your use cases. Specifically, I think it's important to be careful to define the interface to mike in a way that's easy to understand and doesn't produce confusion about what exactly is going to happen under the hood in general, not just for any single scenario. Here, I want to make sure it's as clear as possible that set-default points to a particular identifier (i.e. a subdirectory on the gh-pages branch), not an abstract version (or a "version-and-all-its-aliases") from a deployment.

Anyway, thanks 🙏🏼 if you add an --allow-undefined and/or a --quiet flag on set-default, I'll adjust my cases to it, this will do (Initially I wanted to save you the cost of doing this change yourself with this PR).

--allow-undefined is already merged. I'll think about adding --quiet as a followup.

jimporter added a commit that referenced this pull request Feb 29, 2024
jimporter added a commit that referenced this pull request Feb 29, 2024
@jimporter jimporter closed this in fdcc912 Mar 1, 2024
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.

2 participants