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

Correcting update logic #57

Merged
merged 4 commits into from
Dec 17, 2023

Conversation

kz6fittycent
Copy link
Contributor

@kz6fittycent kz6fittycent commented Dec 16, 2023

I should have stuck with the original logic instead of accepting some proposed changes to the last PR. I should have explained why the logic was the way I wrote it and also thought thru what was being proposed as an alternative before accepting the proposed change; so that's on me. This PR will be the way the logic is used going forward.

The script checks to be sure that the released version from Jenkins is of greater value than what is in the snapcraft.yaml BEFORE it runs a build. If it is of lower value (Jenkins LTS is lower number than latest), then it'll exit. Otherwise, it will build the newest releases.

I should have stuck with the original logic instead of accepting some proposed changes. I should have explained why the logic was the way I wrote it and also thought thru what was being proposed as an alternative before accepting the proposed change. This will be the way going forward.
@merlijn-sebrechts
Copy link
Member

I'm not entirely sure if this is the right approach. If Jenkins quickly releases both a regular version and an lts version before our sync action runs again, then we will not notice the new regular release. Isn't there a way to filter and sort to find the latest non-lts release?

@kz6fittycent
Copy link
Contributor Author

I'm not entirely sure if this is the right approach. If Jenkins quickly releases both a regular version and an lts version before our sync action runs again, then we will not notice the new regular release. Isn't there a way to filter and sort to find the latest non-lts release?

If there is, it's not obvious to me. I can say that this approach does work locally. However, during the last PR, @jnsgruk provided some feedback and I accepted it as part of the PR. I should have stuck with what I'm proposing here; otherwise we end up with Jenkins' latest releases, which are a mix of both LTS, and "latest".

What this is doing is ensuring that their release number is higher than what is in the snapcraft.yaml, if so, it builds, if not, it exits.

@jnsgruk
Copy link
Member

jnsgruk commented Dec 16, 2023

This change looks functionally almost equivalent to me? The reason I suggested the change is there is no reason to have the "dead end" branch that just exits, and it seems cleaner to just have the branch where we actually want the change?

I'm with Merlijn that we should probably find some way to ensure we're getting the LTS - perhaps by parsing this page? https://www.jenkins.io/changelog-stable/

@kz6fittycent
Copy link
Contributor Author

kz6fittycent commented Dec 16, 2023

This change looks functionally equivalent to me?

I'm with Merlijn that we should probably find some way to ensure we're getting the LTS - perhaps by parsing this page? https://www.jenkins.io/changelog-stable/

It's not. So, what we committed yesterday built yet another LTS version (2.426.2) but we've been releasing the latest versions for some time now (2.43+).

This will ensure that the builds we release to candidate are in fact the latest and not the LTS. If we keep doing it the way we've been doing it, we'll be releasing mixed versions constantly. That's not right.

Another point I'd like to make in this is that the LTS versions for Jenkins may work for some, but IMO, the latest provide a better platform.

@popey proposed different channels for the snap, but I mentioned the issues we've had at work with their releases in LTS. We stick with the latest and it works.

@jnsgruk
Copy link
Member

jnsgruk commented Dec 16, 2023

Ah it might just be because of the naive > implementation in bash, which I'm assuming is wrongly coming to the conclusion that the LTS version string is "higher" than latest because there are more chars.

I wonder if -gt or similar behaves better?

(Also sorry for my misunderstanding - I thought we were trying to get the LTSs!)

@kz6fittycent
Copy link
Contributor Author

kz6fittycent commented Dec 16, 2023

Ah it might just be because of the naive > implementation in bash, which I'm assuming is wrongly coming to the conclusion that the LTS version string is "higher" than latest because there are more chars.

I wonder if -gt or similar behaves better?

(Also sorry for my misunderstanding - I thought we were trying to get the LTSs!)

No it doesn't - not in my testing. It just kept giving me errors. I spent some time trying to get this to "just work" and what I've submitted "just works".

For example, this is what we just released earlier to candidate:

channels:
  latest/stable:    2.436   2023-12-15 (4587) 144MB classic
  latest/candidate: 2.426.2 2023-12-16 (4590) 144MB classic
  latest/beta:      ↑                               
  latest/edge:      ↑ 

As you can see, what we've got in stable is the latest release, and the latest candidate is the LTS.

Since I run on the latest all the time, every time these jobs are running, I get reverted back and forth from LTS and latest - irritating lol.

EDIT: I see what you're asking here @jnsgruk - no, what I've tested successfully is that, as the logic stands in this PR, it will ensure that only the latest version, NOT the LTS, is built each time. No more mix-ups, at least in my "hours" of testing.

@jnsgruk
Copy link
Member

jnsgruk commented Dec 16, 2023

Okay, I'll leave Merlijn to catch up on this one.

One final suggestion: we could consider using sort with -V for more accurate comparisons without any additional tools needing to be installed - I've not tested myself but this looks like an interesting approach :) https://stackoverflow.com/a/4024263

@kz6fittycent
Copy link
Contributor Author

kz6fittycent commented Dec 16, 2023

One thing I didn't provide in this PR is that the version info in snapcraft.yaml will need to be manually changed to ensure it's 2.436 for this logic to work, going forward.

EDIT: Just pushed another commit to my fork. This is intentional (see 3309a70).

<<<<<<< candidate
version: '2.435'
=======
version: '2.426.2'
>>>>>>> candidate

This needs to be set to a lower number to ensure the next build triggers as part of this PR.
@merlijn-sebrechts
Copy link
Member

@kz6fittycent let's discuss this on Matrix, I messaged you.

It's working locally - let's rock.
@kz6fittycent kz6fittycent changed the title reverted to correct logic Correcting update logic Dec 16, 2023
Copy link
Member

@merlijn-sebrechts merlijn-sebrechts left a comment

Choose a reason for hiding this comment

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

LGTM!

@merlijn-sebrechts merlijn-sebrechts merged commit 9f16f37 into snapcrafters:candidate Dec 17, 2023
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants