iScsc / iscsc.fr

The iScsc website, build with passion by wannabe devs 🔥
GNU General Public License v3.0
4 stars 12 forks source link

Add subcommands to bump script #71

Closed ctmbl closed 1 year ago

ctmbl commented 1 year ago

This PR removes the old way of using bump.sh, e.g. ./scripts/bump.sh <version> and add three new flags to bump either to patch, minor or major version! with --patch/-p --minor/-m or --major/-M. It also wraps everything into functions for a cleaner script and adds a test that checks that the most recent git tag and the current version are compatible.

amtoine commented 1 year ago

mm @ctmbl

i run either one of the following from clean work tree

./scripts/bump.sh --patch
./scripts/bump.sh --minor
./scripts/bump.sh --major

and i get

[i] Current version is '0.1.1'
[-] 'iScsc/iscsc.fr' remote is not in remote list

but it really looks to be in my remote list :confused:

>_ git remote -v
iscsc   https://github.com/iScsc/iscsc.fr (fetch)
iscsc   https://github.com/iScsc/iscsc.fr (push)
origin  ssh://git@github.com/amtoine/iscsc.fr (fetch)
origin  ssh://git@github.com/amtoine/iscsc.fr (push)

EDIT: should iScsc/iscsc.fr be called origin? if so the error message is misleading :wink:

ctmbl commented 1 year ago

and i get [i] Current version is '0.1.1' [-] 'iScsc/iscsc.fr' remote is not in remote list

but it really looks to be in my remote list $ git remote -v iscsc https://github.com/iScsc/iscsc.fr (fetch) iscsc https://github.com/iScsc/iscsc.fr (push) origin ssh://git@github.com/amtoine/iscsc.fr (fetch) origin ssh://git@github.com/amtoine/iscsc.fr (push)

EDIT: should iScsc/iscsc.fr be called origin? if so there error message is misleading wink

Thanks for raising this issue to me! Actually if you look closely at the code it doesn't expect iscsc remote from being called origin which would be quite unrealiable! But it clearly expect the remote of being a ssh remote and not a https one :smile: Do you think I should change that and accept both? or maybe just raise a warning and don't try to push but let the script run?

amtoine commented 1 year ago

Thanks for raising this issue to me! Actually if you look closely at the code it doesn't expect iscsc remote from being called origin which would be quite unrealiable!

ok perfect :relieved:

But it clearly expect the remote of being a ssh remote and not a https one smile Do you think I should change that and accept both? or maybe just raise a warning and don't try to push but let the script run?

mm i'd say at least say what the exact problem is :+1:

whether to be strict about the git protocol, i'm not sure :thinking: the warning message is a priority, at least to let the user change the remote by hand :wink:

ctmbl commented 1 year ago

@amtoine I improved the log message and avoid the push step in case the remote is not found, tell me what you think of that! I also went with your USAGE proposal.

amtoine commented 1 year ago

@atxr do you approve or do you have a question? :yum:

ctmbl commented 1 year ago

@atxr

Are these errors meaningful for you?

Oh actually yes I get it! As @amtoine highlighted it before the problem comes from the script not finding the iScsc remote, you can see the warning above: [~] 'git@github.com:iScsc/iscsc.fr.git' remote is not in remote list, won't push Before last commit this would end up exiting the script but I changed it thinking that stepping over the push stage would be sufficient, but I was wrong this is not the only time the variable ISCSC_REMOTE is used. It is in the git_setup function and because it is empty, the 2nd argument BUMP_BRANCH is interpreted as the first one and here we understand these confusing error message. I'll just revert last commit to exit the script in case the ISCSC_REMOTE is empty, and allow https remotes @amtoine

ctmbl commented 1 year ago

@amtoine I also noted something strange:

$_ git remote -v iscsc https://github.com/iScsc/iscsc.fr (fetch) iscsc https://github.com/iScsc/iscsc.fr (push) origin ssh://git@github.com/amtoine/iscsc.fr (fetch) origin ssh://git@github.com/amtoine/iscsc.fr (push)

Did you modified your git remote -v output? I get

$ git remote -v
fork    git@github.com:ctmbl/iscsc.fr.git (fetch)
fork    git@github.com:ctmbl/iscsc.fr.git (push)
origin  git@github.com:iScsc/iscsc.fr.git (fetch)
origin  git@github.com:iScsc/iscsc.fr.git (push)

no ssh:// before my ssh cloned remote and .git at the end Note that this is what GitHub provides when cloning https://github.com/iScsc/iscsc.fr (Code green button)

atxr commented 1 year ago

It's weird because I'm still getting an error even if iScsc/iscsc.fr is in the remote list :thinking:

[-] 'iScsc/iscsc.fr' remote is not in remote list
ctmbl commented 1 year ago

It's weird because I'm still getting an error even if iScsc/iscsc.fr is in the remote list thinking

At least it's better than messing up with git on the wrong branch!

amtoine commented 1 year ago

@ctmbl

i added the remote as they show up in the git remote -v i think

i don't know, it's quite old now :grimacing:

in the end, i think the ssh:// is implicit when you have git@github.com, i'd say git knows about that :+1:

ctmbl commented 1 year ago

@amtoine

in the end, i think the ssh:// is implicit when you have git@github.com, i'd say git knows about that +1

Actually that wasn't my point at all, and I'm much more worried about the .git that you haven't If we have different git remote -v the grep won't work that's the problem... So why do our outputs are f*cking different?

amtoine commented 1 year ago

@ctmbl

If we have different git remote -v the grep won't work that's the problem...

you can try to match for any remote containing iscsc/iscsc.fr. or you can just git remote add bump <the remote you want> and with that instead and never use git remote -v. that might be the easiest :+1:

So why do our outputs are f*cking different?

you can git remote add anything, both for the remote name and the remote location. that won't be a problem for git until you actually try to interact with upstream.

e.g.

git remote add djwaldj djhcklnzsc

would work just fine

ctmbl commented 1 year ago

@amtoine

Answers

you can try to match for any remote containing iscsc/iscsc.fr.

I was thinking of that, but that doesn't solve the upper/lower case problem, it has been managed by --ignore-case flag added to each grep but I don't like it... quite error-prone while maintaining the script. However it does solve other problems

or you can just git remote add bump and with that instead and never use git remote -v. that might be the easiest 👍

The easiest yes but here are the reasons why I need this remote:

While the 2nd step isn't necessary I find the first one quite mandatory, then: If I was to follow your solution (adding a bump remote) I'd have to:

Quite an overhead in my opinion, I'd prefer to use the existing remotes and let the user deal with them in the way he likes it 👍

you can git remote add anything, both for the remote name and the remote location. that won't be a problem for git until you actually try to interact with upstream.

Yes ofc but I don't see how this is relevant... or did you add garbage remotes? I implicitely assumed that what was in your remotes worked, then, why does it work with different remotes? didn't you just copy/paste the remotes given by GitHub when cloning or adding new remotes? Well in any case it works so I have to deal with it 👍

Summary

For the reasons stated in my 2nd answer I think I'll go with the existing remotes in the user remotes, I'll just have to carefully deal with edge cases as you have. The good thing is, we're three and we have three different remotes, it probably gives me every different possibilities the script may encounter and this is useful to highlight bugs now and not when this PR will be merged, so even if it pisses me off thanks guys 👍

amtoine commented 1 year ago

Yes ofc but I don't see how this is relevant... or did you add garbage remotes? I implicitely assumed that what was in your remotes worked, then, why does it work with different remotes?

these are not "garbage" remotes, they work just fine

  • ^ssh:// is optional when you already have git@github.com
  • .git$ is optional

didn't you just copy/paste the remotes given by GitHub when cloning or adding new remotes?

i know the pattern of these git remotes, so i do not always copy paste them :+1:

ctmbl commented 1 year ago

these are not "garbage" remotes, they work just fine

Ofc they aren't in your case because you're using them! I was caricaturing because you previously showed a garbage one

^ssh:// is optional when you already have git@github.com .git$ is optional

Ok this is interesting, annoyting and cool at the same time I should say

atxr commented 1 year ago

Thanks for solving this bug @ctmbl You can safely merge now :wink: