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

Fix badge status #2991

Merged
merged 1 commit into from
Jan 7, 2023
Merged

Fix badge status #2991

merged 1 commit into from
Jan 7, 2023

Conversation

novialriptide
Copy link
Contributor

No description provided.

rejas
rejas previously approved these changes Jan 5, 2023
Copy link
Collaborator

@rejas rejas left a comment

Choose a reason for hiding this comment

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

Probably best to merge it into master like the PR wants to. Any objections @MichMich ?

@khassel
Copy link
Collaborator

khassel commented Jan 7, 2023

should be available on master asap.

We could merge this on develop as usual and then cherry pick this commit on master.

@rejas
Copy link
Collaborator

rejas commented Jan 7, 2023

Sounds like a plan. Do you want to?

@khassel
Copy link
Collaborator

khassel commented Jan 7, 2023

Sounds like a plan. Do you want to?

o.k.

@khassel khassel changed the base branch from master to develop January 7, 2023 19:36
@khassel khassel changed the base branch from develop to master January 7, 2023 19:37
@khassel khassel dismissed rejas’s stale review January 7, 2023 19:37

The base branch was changed.

@khassel khassel merged commit 2eaf9df into MagicMirrorOrg:master Jan 7, 2023
khassel added a commit that referenced this pull request Jan 7, 2023
@khassel
Copy link
Collaborator

khassel commented Jan 7, 2023

did it the other way (merge on master, cherry pick on develop) because I had 162 changes in the web ui after changing base to develop ...

@MichMich
Copy link
Collaborator

MichMich commented Jan 8, 2023

Sorry for my slow response. My bad. But for future PR's I think we should refrain from any merge into master if it's not solving a blocking issue.

This PR into master caused an update notification for all users which seem to have caused a lot of confusion (especially since it's not a tagged commit).

I really try to limit any update to master to the four releases per year to prevent update-fatigue. :)

@rejas
Copy link
Collaborator

rejas commented Jan 9, 2023

shall we add the "never ever merge to master" rule to our Collaboration.md @sdetweil @khassel ?

@khassel
Copy link
Collaborator

khassel commented Jan 9, 2023

and "only MichMich does releases" ...

I think I have a solution for "never ever merge to master". I updated the updatenotification module (still draft)

  • if repo = MagicMirror repo
  • and branch = master
  • and behind head of master
  • check if there is a tag on the commits between current commit and head of master
  • and only send a notification if so

This would allow us to update things on master without user getting notified.

@sdetweil
Copy link
Collaborator

i think never commit to master unless it is a hotfix and changes the version number and is tagged

certainly no reason to update the readme

@rejas
Copy link
Collaborator

rejas commented Jan 11, 2023

and "only MichMich does releases" ...

I think I have a solution for "never ever merge to master". I updated the updatenotification module (still draft)

* if repo = MagicMirror repo

* and branch = `master`

* and behind head of `master`

* check if there is a tag on the commits between current commit and head of `master`

* and only send a notification if so

This would allow us to update things on master without user getting notified.

Nice idea, but this has got to be very foolproof and well tested. Imagine the update notifications being never shown again and therefore no bugfixes will get pulled...

So maybe just go the "never merge to master unless a hotfix with a new version tag is necessary" route?

@khassel
Copy link
Collaborator

khassel commented Jan 11, 2023

So maybe just go the "never merge to master unless a hotfix with a new version tag is necessary" route?

thats the current situation (at least until next release) ...

but this has got to be very foolproof and well tested

agree, did already some testing. Will do a draft PR (not yet, depends on another upcoming PR) and the you can take a look ...

@novialriptide
Copy link
Contributor Author

novialriptide commented Jan 12, 2023

This PR into master caused an update notification for all users which seem to have caused a lot of confusion (especially since it's not a tagged commit).

@MichMich

I'm confused, does this mean everyone using MagicMirror2 has received a notification about this pull request?

@sdetweil
Copy link
Collaborator

yes. the updatenotification module was checking commits, and this one was a+1, so it popped up a change notice on every mirror running the notification module.

and my update script failed cause the version didn't change.

@novialriptide
Copy link
Contributor Author

yes. the updatenotification module was checking commits, and this one was a+1, so it popped up a change notice on every mirror running the notification module.

and my update script failed cause the version didn't change.

I feel famous

@khassel
Copy link
Collaborator

khassel commented Jan 12, 2023

I feel famous

me too, because I did the push 😚

rejas added a commit that referenced this pull request Jan 26, 2023
)

see discussion here:
#2991 (comment)

I still see a need for updating `master` in special cases (e.g. correct
errors in README.md which would otherwise be present up to 3 month until
next release) so with this PR **only for MagicMirror repo** and **only
for `master` branch** updatenotifications are only triggered if at least
one of the new commits has a tag.

May @MichMich must decide if this is wanted.

Co-authored-by: Veeck <github@veeck.de>
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.

5 participants