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

Added automation for light hotspots #414

Merged
merged 3 commits into from
May 5, 2022

Conversation

MuratUrsavas
Copy link
Contributor

@MuratUrsavas MuratUrsavas commented Mar 30, 2022

Also removed necessity for upfront I2C device report. Now a single release could be used for all of the devices.

Issue
(#399)

  • Summary:

How

  • Removed upfront device report necessity
  • Escalated gatewayrs container privileges
  • Removed per device type fleet for testnets
  • Removed all matrices and made deployment workflows simpler
  • Added documentation for necessary IP ports
  • Removed all docker-compose outputs as they always needed to be created again and creating unnecessary steps in workflows

Checklist

  • Tests added
  • Cleaned up commit history (rebase!)
  • Documentation added
  • Thought about variable and method names

@MuratUrsavas MuratUrsavas requested review from a team as code owners March 30, 2022 12:07
@shawaj
Copy link
Member

shawaj commented Mar 30, 2022

Removed all docker-compose outputs as they always needed to be created again and creating unnecessary steps in workflows

i think it is useful to keep these, so we can easily see from github the latest version (as well as the fact we need to push that to the relevant balena openfleets directories)

also, i think we need to keep device arch (the reason we added that is to support 5g in here further down the line which uses amd64)

the only way around that would be to put arch in hm-pyhelper and use it as part of the workflows in this repo as per #336

templates/docker-compose.template Outdated Show resolved Hide resolved
.github/workflows/update-gatewayrs.yml Show resolved Hide resolved
.github/workflows/push-to-testnet-draft.yml Show resolved Hide resolved
templates/docker-compose.template Show resolved Hide resolved
settings.ini Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@MuratUrsavas
Copy link
Contributor Author

i think it is useful to keep these, so we can easily see from github the latest version (as well as the fact we need to push that to the relevant balena openfleets directories)

I'm against keeping any automated output files in the repo. IMHO these are just unnecessary steps making things complicated. Also they include old information (previous commit). If any developer changes something locally and pushes manually to a fleet, the information would be incorrect. Without a compose file, any of a new developer would understand the necessity to create one and refer to readme.

Long story short, I'm still on the remove side.

also, i think we need to keep device arch (the reason we added that is to support 5g in here further down the line which uses amd64)

the only way around that would be to put arch in hm-pyhelper and use it as part of the workflows in this repo as per #336

As of now nothing was referencing to the architecture. So removing the extra information from settings.ini made things a lot simpler. If we'd need an architecture variation some day, we'd solve it like your suggestion (#336)

@MuratUrsavas MuratUrsavas force-pushed the murat/add-light-hotspot-automation branch from 1d10365 to 585e062 Compare March 31, 2022 07:23
@shawaj
Copy link
Member

shawaj commented Mar 31, 2022

If any developer changes something locally and pushes manually to a fleet, the information would be incorrect.

We shouldn't be pushing manually to testnet or production fleets IMO.

Also, having a quick reference for debugging rather than having to generate one from a python script is very useful when errors arise.

And we need them for the other balena OpenFleets repos so I don't see any way around keeping them.

As of now nothing was referencing to the architecture. So removing the extra information from settings.ini made things a lot simpler. If we'd need an architecture variation some day, we'd solve it like your suggestion (#336)

Sure, for lights it's maybe not relevant as it won't be referenced.

We will need the arch stuff for 5g very soon though so we should bump that up.

@pritamghanghas
Copy link
Contributor

If any developer changes something locally and pushes manually to a fleet, the information would be incorrect.

We shouldn't be pushing manually to testnet or production fleets IMO.

Also, having a quick reference for debugging rather than having to generate one from a python script is very useful when errors arise.

And we need them for the other balena OpenFleets repos so I don't see any way around keeping them.

As of now nothing was referencing to the architecture. So removing the extra information from settings.ini made things a lot simpler. If we'd need an architecture variation some day, we'd solve it like your suggestion (#336)

Sure, for lights it's maybe not relevant as it won't be referenced.

We will need the arch stuff for 5g very soon though so we should bump that up.

I also think build time access to arch might come in handy with dockerfiles down the line.

@MuratUrsavas
Copy link
Contributor Author

I also think build time access to arch might come in handy with dockerfiles down the line.

The main idea is, add more complexity when it's needed. Not upfront.

@MuratUrsavas
Copy link
Contributor Author

Also, having a quick reference for debugging rather than having to generate one from a python script is very useful when errors arise.

And we need them for the other balena OpenFleets repos so I don't see any way around keeping them.

As of now nothing was referencing to the architecture. So removing the extra information from settings.ini made things a lot simpler. If we'd need an architecture variation some day, we'd solve it like your suggestion (#336)

Sure, for lights it's maybe not relevant as it won't be referenced.

We will need the arch stuff for 5g very soon though so we should bump that up.

The next stop is changing the start script to full python and implement all of these with the help of hm-pyhelper. But this ticket is long overdue, it will be nice to move it to testnet and then tackle with all of those in the next step.

Murat Ursavas added 2 commits April 29, 2022 13:53
…y for every device type.

Removed necessity for upfront I2C device report. Now a single release could be used for all of the devices.
(#399)
@MuratUrsavas MuratUrsavas force-pushed the murat/add-light-hotspot-automation branch from 938dcd2 to 6f85b8b Compare April 29, 2022 10:54
Copy link
Contributor

@kashifpk kashifpk left a comment

Choose a reason for hiding this comment

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

@MuratUrsavas - looks good to me.

@shawaj
Copy link
Member

shawaj commented Apr 29, 2022

Looking again now. Sorry for delay

@shawaj
Copy link
Member

shawaj commented Apr 30, 2022

The next stop is changing the start script to full python and implement all of these with the help of hm-pyhelper. But this ticket is long overdue, it will be nice to move it to testnet and then tackle with all of those in the next step.

Yeah makes sense. We can always change before merging gateway-rs stuff to mainline.

@MuratUrsavas all looks good to me other than the failing unit tests

@MuratUrsavas
Copy link
Contributor Author

@MuratUrsavas all looks good to me other than the failing unit tests

The test failures showed up after the rebase. Those additions belong to @pritamghanghas so I didn't touch them to not to break anything.

@MuratUrsavas MuratUrsavas merged commit 870f7ed into light-hotspot May 5, 2022
@MuratUrsavas MuratUrsavas deleted the murat/add-light-hotspot-automation branch May 5, 2022 08:03
@pritamghanghas
Copy link
Contributor

@MuratUrsavas has to related to version change. It was working till about a couple of days back.

@@ -93,4 +87,3 @@ networks:
ipam:
driver: default
config:
- subnet: 172.20.0.0/16
Copy link
Contributor

Choose a reason for hiding this comment

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

@MuratUrsavas removal of this line broke docker-compose. Please add it back.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Arghh. @pritamghanghas it should be left as like this while testing. Didn't mean to remove that. Will open a quick PR.

@MuratUrsavas MuratUrsavas mentioned this pull request May 5, 2022
4 tasks
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.

4 participants