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.

Sign up for GitHub

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

Jump to bottom

Update controller to 8.1.113 #495

Merged
merged 4 commits into from
Mar 22, 2024
Merged

Update controller to 8.1.113 #495

merged 4 commits into from
Mar 22, 2024

Conversation

cwmoriarty
Copy link
Contributor

Proposed Changes

Update controller to 8.1.133 (current). Tested locally.

@cwmoriarty
Copy link
Contributor Author

FWIW I have not yet tested this with the changes in cfergeau:base9.

@cfergeau
Copy link
Contributor

FWIW I have not yet tested this with the changes in cfergeau:base9.

In my opinion it's best to get this in first and make a release as this PR is trivial compared to mine :)

@renewoensdregt
Copy link
Contributor

@cwmoriarty You beat me to it, but this build failed because 17.98 E: Version '2.34-6ubuntu1.8' for 'binutils' was not found

You need to change it to 2.34-6ubuntu1.9

@frenck
Copy link
Member

frenck commented Mar 20, 2024

You need to change it to 2.34-6ubuntu1.9

Not really in this PR though....

@cwmoriarty
Copy link
Contributor Author

@cwmoriarty You beat me to it, but this build failed because 17.98 E: Version '2.34-6ubuntu1.8' for 'binutils' was not found

You need to change it to 2.34-6ubuntu1.9

Definitely support merging your PR first.

@renewoensdregt
Copy link
Contributor

Maybe I am not clear on full git processes yet. Would you not include all changes in one PR to make the new version work? If this PR is merged as-is, it will not work. I can submit a fully working PR soon I think with both changes, but if that is not the right process, let me know. Learning as we go.

@cwmoriarty
Copy link
Contributor Author

Maybe I am not clear on full git processes yet. Would you not include all changes in one PR to make the new version work? If this PR is merged as-is, it will not work. I can submit a fully working PR soon I think with both changes, but if that is not the right process, let me know. Learning as we go.

I think best practice here is to sync your branch with mine and resolve the changes. I have added the correct binutils pin.

@frenck
Copy link
Member

frenck commented Mar 20, 2024

Would you not include all changes in one PR to make the new version work?

It isn't related.

@renewoensdregt
Copy link
Contributor

renewoensdregt commented Mar 20, 2024 via email

@cfergeau
Copy link
Contributor

Would you not include all changes in one PR to make the new version work?

It isn't related.

It's definitely not related, but since it's separated in its own commit, and since there aren't multiple branches in flux which would need this change, then it's in my opinion acceptable and less cumbersome for the contributor to have this fix together with a PR doing something different.

@frenck
Copy link
Member

frenck commented Mar 20, 2024

then it's in my opinion acceptable

Sorry, I don't agree. So, there is that.

../Frenck

frenck
frenck reviewed Mar 20, 2024
View reviewed changes
unifi/Dockerfile Outdated Show resolved Hide resolved
@renewoensdregt
Copy link
Contributor

Sorry, I don't agree. So, there is that.
Time to use my favorite quote at work.... "You´re not paid to like it "

@frenck frenck added the dependencies Upgrade or downgrade of project dependencies. label Mar 20, 2024
@cwmoriarty
Copy link
Contributor Author

@frenck What's next here?

@renewoensdregt
Copy link
Contributor

renewoensdregt commented Mar 21, 2024 via email

@renewoensdregt
Copy link
Contributor

renewoensdregt commented Mar 21, 2024 via email

@frenck
Copy link
Member

frenck commented Mar 22, 2024

@renewoensdregt You really need to have more patience...

@frenck
Copy link
Member

frenck commented Mar 22, 2024 •
edited
Loading

@frenck What's next here?

Wait? There is no fire to get this out in a rush?

Please.... this is a simple bump and already has 16 comments... this is not healthy....

@renewoensdregt
Copy link
Contributor

renewoensdregt commented Mar 22, 2024 •
edited
Loading

@frenck There is no fire, but please understand that not everyone is as clear on Git and the processes as you are. I´m learning and really want to help, but if there is no explanation, clarification on what to do vs just a comment that you disagree, that does not really help me to do what you expect needs to be done. Little bit of empathy would be appreciated :-)

@frenck
Copy link
Member

frenck commented Mar 22, 2024 •
edited
Loading

Little bit of empathy would be appreciated :-)

This is not a school. There are tons of resources out there to learn this stuff.

Please note, all these reactions only cause noise, notifications and helps really nobody. If anything, It make me mute PRs/issues to get rid of the noise (and thus has the opposite effect).

frenck
frenck approved these changes Mar 22, 2024
View reviewed changes
Copy link
Member

@frenck frenck left a comment

Choose a reason for hiding this comment

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

Thanks, @cwmoriarty 👍

../Frenck

@frenck frenck merged commit e3180c4 into hassio-addons:main Mar 22, 2024
10 checks passed
@cfergeau
Copy link
Contributor

Little bit of empathy would be appreciated :-)

This is not a school. There are tons of resources out there to learn this stuff.

@renewoensdregt is not clear on the processes expected to be used in addon-unifi, and hassio-addons in general, which are - as far as I understand - based on how you work. I'm not sure this is explicitly documented? I had the same confusion regarding your expectations for the binutils bump.

In general, when people understand the processes, they can file PRs in the right way from the start, resulting in

  • less work for you because you did not need to do the changes/testing/... yourself
  • less PR review work/less discussions, because people were aware of how things should be when filing the PR
  • ideally, they can even help review other PRs so that they get in the shape you expect

@frenck
Copy link
Member

frenck commented Mar 22, 2024

I'm not sure this is explicitly documented?

Keep PR small and to the point. That is a general rule in software development and doing PR anywhere. You should not need documentation for that.

n general, when people understand the processes, they can file PRs in the right way from the start, resulting in

You are making this way bigger and complex than it should be. Also, again, noise.

STOP IT.

Thanks 👍

@buzo-ffm
Copy link

Minor detail: The version number is 8.1.113, not 133, and these are the release notes.

@cwmoriarty cwmoriarty changed the title Update controller to 8.1.133 Update controller to 8.1.113 Mar 22, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Mar 24, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@frenck frenck frenck approved these changes

Assignees
No one assigned
Labels
dependencies Upgrade or downgrade of project dependencies.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@cwmoriarty @cfergeau @renewoensdregt @frenck @buzo-ffm