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

Add Nebra software version #290

Closed
shawaj opened this issue Nov 4, 2021 · 36 comments
Closed

Add Nebra software version #290

shawaj opened this issue Nov 4, 2021 · 36 comments
Assignees

Comments

@shawaj
Copy link
Member

shawaj commented Nov 4, 2021

We need to add this to diagnostics once implemented...

NebraLtd/hm-miner#47

@shawaj shawaj changed the title As Nebra software version Add Nebra software version Nov 4, 2021
@MuratUrsavas
Copy link
Contributor

Since this is the very first task of my new role I'd like to check my understanding:

In the current state hm-diag does not report the version of the Nebra firmware and we'd like to get it in that report. Is this correct? Will that be the version of helium-miner-software or hm-diag's? How do we track our releases?

@shawaj
Copy link
Member Author

shawaj commented Jan 19, 2022

@marvinmarnold think maybe we were going to scrap this as we are now just appending a number to the Helium firmware version and we have the settings.ini from the main repo?

Unless we want to expose all of the versions from settings.ini as ENV variables into the diagnostics container perhaps?

@MuratUrsavas
Copy link
Contributor

I'm seeing an ENV variable in docker-compose file. I think this is what we'd like to report back.

I see that hm-diag version doesn't match to helium-miner-software's version. and the most logical one would be to report helium-miner-software's. What do you think?

@MuratUrsavas
Copy link
Contributor

MuratUrsavas commented Jan 19, 2022

@marvinmarnold think maybe we were going to scrap this as we are now just appending a number to the Helium firmware version and we have the settings.ini from the main repo?

Unless we want to expose all of the versions from settings.ini as ENV variables into the diagnostics container perhaps?

I don't see a settings.ini file in the main repo. Is that created dynamically on first run or carried by another repo?

Sorry, these are really silly questions, but please bare with me while I try to explore everything :)

Edit Note: Found my answer in PR helium-miner-software#341.

@shawaj
Copy link
Member Author

shawaj commented Jan 19, 2022

@MuratUrsavas at the moment we have the firmware version in diagnostics already and it takes the format from helium version such as 2022.01.12.0 - if we make updates between helium miner releases of the firmware we then append an increment number like -1 or -2to the the to give something like 2022.01.12.0-2

What I'm not sure of is whether @vpetersson and @marvinmarnold think it's still relevant to have a separate Nebra version number and helium firmware version or whether we could keep this incrementing way.

Alternatively / as well as this we could host use the short SHAs in the settings.ini file for each container version perhaps.

Not sure exactly what is best

@MuratUrsavas
Copy link
Contributor

MuratUrsavas commented Jan 19, 2022

Thanks for the explanation Aaron. I thought the same. One readable version in the main repo should be enough. We can pull the SHA's of the sub repos at build time (need to check the possibility). IMHO we don't need to track every sub repo individually with readable versions.

I'll wait for a consensus on this issue.

Edit Note: We're using the SHA in the "image" variable anyway. I think that variable could be used for version tracking / reporting for the sub repos.

@shawaj
Copy link
Member Author

shawaj commented Jan 19, 2022

Yeah I agree I'm not sure there's much merit in having all the SHAs. The end user doesn't really care and we can already cross reference back to them.

FYI in helium-miner-software repo the files are built and stored in device-compose-files folder and this is what is pushed to balena. So not sure there's any real need to do anything else with that particularly

But yeah maybe just see what @marvinmarnold and @vpetersson think as there may be something I'm not remembering here

@vpetersson
Copy link
Contributor

So what I'm thinking is we do something like $HELIUM_VERSION ($SHORT_GIT_HASH). It's fairly common to report versions like that so I don't think it will cause any confusion to end users. The added benefit with adding the git hash is that we can pinpoint to the exact commit the device is running (as we have historically bumped say hm-diag w/out bumping the helium version). Do however note that this is the git hash from helium-miner-software rather than the hm-diag repo, which in turn then points to a distinct git hash across all included repos.

You might need to check with @SebastianMaj or @kashifpk to ensure that we export this as an environment variable in the docker-compose file generated in helium-miner-software.

@MuratUrsavas
Copy link
Contributor

FYI in helium-miner-software repo the files are built and stored in device-compose-files folder and this is what is pushed to balena. So not sure there's any real need to do anything else with that particularly

@shawaj Excuse my ignorance but I can't see this folder anywhere. Obviously "balena push" builds the image on balena-cloud. Therefore I tried to build the image locally with "balena build" command, switched the device to local mode but yet to see it. Neither on my dev machine nor on my test device.

@vpetersson
Copy link
Contributor

@MuratUrsavas so i wouldn't recommend using 'local mode' as it doesn't exactly mimic the remote setup. What we do instead is to create a dedicated developer fleet for you that you can use.

@MuratUrsavas
Copy link
Contributor

@shawaj Excuse my ignorance but I can't see this folder anywhere. Obviously "balena push" builds the image on balena-cloud. Therefore I tried to build the image locally with "balena build" command, switched the device to local mode but yet to see it. Neither on my dev machine nor on my test device.

Things are moving really fast. Now I know why ı didn't see it. Master does not look like the same as on Monday :) I had forked it as usual and was taking a look with it. Now deleted the fork and cloned the original repo as said in the onboarding doc.

@MuratUrsavas
Copy link
Contributor

I'm planning to add version information as FIRMWARE_VERSION=2022.01.19.1-2 (01dc3e2) when we run the gen_docker_compose.py as a single ENV variable and then propagate it via hm-diag. What do you think? Or should we use two distinct variables for readable version and short SHA?

@vpetersson
Copy link
Contributor

I'd brake them into two ENV variables instead.

@marvinmarnold
Copy link
Contributor

I also think it should be two separate values. I also think the Nebra version should be based on semantic versioning like Aaron suggested in the past. We would then stop appending -X to the FIRMWARE_VERSION for subsequent releases on the same Helium version. This is pretty similar to what Viktor is suggesting with $SHORT_GIT_HASH.

Related to: NebraLtd/hm-pyhelper#20

@MuratUrsavas
Copy link
Contributor

OK, two values then.

So should we implement semantic versioning in this issue or leave it to another one? Since I'm pretty newbie, I'd leave it to a new issue :) I need to figure out how to bump patch and prerelease version numbers.

@marvinmarnold
Copy link
Contributor

Sure, $SHORT_GIT_HASH is fine for now. When you are done, can you make the new ticket for semantic versioning and other related follow up stuff?

@MuratUrsavas
Copy link
Contributor

Yep, sure thing!

@MuratUrsavas
Copy link
Contributor

I thought that adding "xxxxxxx-dirty" flag could be nice to check if someone has done something manually on that particular release (say a newcomer developer like me). For this I need to check the dirtiness status of the repo and done it easily in a git installed environment. (not libgit2).

First question: is git already installed on "ubuntu-latest" machine in the pipeline?

Second question: If so, then it would be better to keep all the logic inside of gen_docker_compose.py as it is called in many different workflows. It will be the only source of information and would work on local setups flawlessly. What do you guys think?

@vpetersson
Copy link
Contributor

It's impossible to get a dirty state in GitHub Actions as it just pulls from upstream.

First question: is git already installed on "ubuntu-latest" machine in the pipeline?

If you ever feel the need to invoke git, you're approaching the problem the wrong way. You should be able to read all relevant data from Actions directly. Do check out this page.

If so, then it would be better to keep all the logic inside of gen_docker_compose.py as it is called in many different workflows. It will be the only source of information and would work on local setups flawlessly. What do you guys think?

Yes, you will need to build this into gen_docker_compose.py. Read the environment variables and pass them onto the template. It's really straight forward.

All you need to do is:

  • Update gen_docker_compose.py to read the relevant environment variables (as set by GitHub Actions metadata)
  • Populate two new environment variables in the Docker Compose template for hm-diag (say 'VERSIONandGIT_HASH`)
  • Read them back in hm-diag

Should really be rather straight forward. No need to manually interact with git at all.

@MuratUrsavas
Copy link
Contributor

@vpetersson The basic logic was ready. Then I thought it would be nice to see the dirtiness. As you've mentioned it pulls from upstream and the environment would never be dirty.

OK, I'll continue with github.actions ENV variable and will document a method to test this locally with dirtiness information.

@vpetersson
Copy link
Contributor

I wouldn't worry about dirtiness. We never push to testnet or production from a local environment.

@MuratUrsavas
Copy link
Contributor

Even balena push uses a commit, right? Not the working copy.

@vpetersson
Copy link
Contributor

See balena-cli-action. That's what is used for pushing to Balena. Basically, it will use the docker-compose.yml file you point it to, which in turn pulls down images as per the hash specified.

@MuratUrsavas
Copy link
Contributor

Thanks Viktor. My only concern was to test them on our test fleets locally under personal Balena accounts. But as you've mentioned grouping our test devices under Nebra would be a better idea.

@vpetersson
Copy link
Contributor

I'm less worried about testing this particular change as it is fairly simple change.

@MuratUrsavas
Copy link
Contributor

First part is ready. Now it's time to use it in hm-diag reports.

@MuratUrsavas
Copy link
Contributor

MuratUrsavas commented Jan 24, 2022

is /version information enough or do I have to include this also under /json and /routes?

Planning to put it everywhere where FIRMWARE_VERSION parameter exists.

@vpetersson
Copy link
Contributor

Probably enough to do it in /version as I think HPT is pulling that anyways. Please confirm @kashifpk

@kashifpk
Copy link
Contributor

Probably enough to do it in /version as I think HPT is pulling that anyways. Please confirm @kashifpk

Yes @vpetersson, HPT reads this info and also logs it on screen and to syslog (hence in logtail too).

@vpetersson vpetersson transferred this issue from another repository Jan 24, 2022
@vpetersson vpetersson transferred this issue from NebraLtd/helium-miner-software Jan 24, 2022
@marvinmarnold
Copy link
Contributor

FYI, seems related to this: https://engineering.helium.com/2022/01/24/firmware-release-containerized-miner.html. Haven't dug into it much but we should be able to change the miner version independently of our firmware.

@shawaj
Copy link
Member Author

shawaj commented Jan 24, 2022

Do we want to auto update the miner aromatically though? We have had various incidences of breaking changes not properly reported etc so not sure if maybe it's better not to?

@vpetersson
Copy link
Contributor

I'm not in favor of auto updating. There has been way too many breaking changes in the past. Now that we will be doing various things over RPC etc, we need predictable behavior.

@marvinmarnold
Copy link
Contributor

Agreed, not in favor of auto-updating. Just meant that there's an established pattern for updating miner independently of firmware.

@MuratUrsavas
Copy link
Contributor

Probably enough to do it in /version as I think HPT is pulling that anyways. Please confirm @kashifpk

I had been already added the information where the FIRMWARE_VERSION is reported. The PR's are ready.

NebraLtd/helium-miner-software#350
#291

The tests are OK thanks to @ilyastrodubtsev.

If you can approve them, we can close this ticket and follow-up the rest in NebraLtd/helium-miner-software#351.

@MuratUrsavas
Copy link
Contributor

Asking for the next sprint: We don't think using semantic auto versioning is better than what we have (date based versions) right now, hm-miner would get updated manually to fetch upstream changes and the release procedure would be kept as usual.

Am I correct?

@MuratUrsavas
Copy link
Contributor

@vpetersson & @marvinmarnold Should we close this issue with the merge of NebraLtd/helium-miner-software#350 and then discuss the new version style in NebraLtd/helium-miner-software#351?

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

No branches or pull requests

5 participants