Skip to content

Conversation

@victormlg
Copy link
Contributor

Added a way to update modules added by url. I tried to do it with minimal extra code inside the update loop. I am not sure if we want to delete the older commit/repo

@victormlg victormlg force-pushed the update_module_by_urls branch from bcdd104 to 9b6ee91 Compare June 4, 2025 11:48
@victormlg victormlg requested a review from jakub-nt June 4, 2025 11:48
@victormlg victormlg force-pushed the update_module_by_urls branch 3 times, most recently from a314117 to c7a9df6 Compare June 10, 2025 09:02
@victormlg victormlg requested a review from jakub-nt June 10, 2025 09:03
@victormlg victormlg force-pushed the update_module_by_urls branch from 7bed220 to 9a2cc97 Compare June 10, 2025 14:44
@victormlg victormlg requested review from jakub-nt and larsewi June 10, 2025 14:45
Copy link
Contributor

@larsewi larsewi left a comment

Choose a reason for hiding this comment

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

Please add shell tests for updating module (added with URL). Also include test cases where:

  • steps changed
  • input changed

When running cfbs update on the following project

$ cfbs status
Name:        Example project
Description: Example description
File:        cfbs.json

Modules:
001 masterfiles               @ 3.24.2 / 5c49f1ed1c3a4ecfc46b4b61296ff5dd38991f33 (Downloaded)
002 compliance-report-imports @        / 1a08bf82757326039e1c1836f8e9ff064f4a9e44 (Downloaded)

It says that masterfiles is already up-to-date. But it does not mention compliance-report-imports (added with URL).

$ cfbs update
Module 'masterfiles' already up to date
Modules are already up to date

@victormlg victormlg force-pushed the update_module_by_urls branch 2 times, most recently from 1eeaee0 to 8736e7b Compare June 23, 2025 13:09
@olehermanse olehermanse requested a review from larsewi June 23, 2025 14:22
@larsewi
Copy link
Contributor

larsewi commented Jun 24, 2025

Looks good, but please add some shell tests. You can for example add this module using URL, modify the commit SHA, input, build steps and run cfbs update to see that it works similar to modules added through the normal index.

@victormlg victormlg force-pushed the update_module_by_urls branch from 49e7aff to dec5050 Compare June 24, 2025 12:35
Copy link
Contributor

@larsewi larsewi left a comment

Choose a reason for hiding this comment

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

Great work 🚀

@olehermanse olehermanse self-requested a review June 24, 2025 13:51
Copy link
Member

@olehermanse olehermanse left a comment

Choose a reason for hiding this comment

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

Please rebase on latest master, rename your test with a new number (39 is no longer the next number since other tests have been added) and add your test to all.sh

@victormlg victormlg force-pushed the update_module_by_urls branch from dec5050 to ada3713 Compare June 24, 2025 14:23
@victormlg victormlg requested a review from olehermanse June 24, 2025 14:23
@olehermanse olehermanse requested a review from larsewi June 25, 2025 12:29
@victormlg victormlg force-pushed the update_module_by_urls branch 2 times, most recently from 7fbd884 to dec5050 Compare June 26, 2025 14:30
Copy link
Member

@olehermanse olehermanse left a comment

Choose a reason for hiding this comment

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

It looks like you made a mistake and went back in time here - Your shell test needs to be renamed (again) and added to all.sh.

@larsewi
Copy link
Contributor

larsewi commented Jun 27, 2025

It looks like you made a mistake and went back in time here - Your shell test needs to be renamed (again) and added to all.sh.

This is why you should use --force-with-lease instead of -f when pushing

The part of the loop in update_command responsible for actually updating modules was moved to its own function. It is done by passing a module updates class that tracks the updates generally.

Signed-off-by: Victor Moene <victor.moene@northern.tech>
@victormlg victormlg force-pushed the update_module_by_urls branch from dec5050 to 3f1eee9 Compare June 30, 2025 11:40
@victormlg victormlg force-pushed the update_module_by_urls branch 4 times, most recently from c549d86 to b03caa8 Compare June 30, 2025 12:22
@victormlg victormlg requested a review from olehermanse June 30, 2025 12:24
Ticket: CFE-3847
Signed-off-by: Victor Moene <victor.moene@northern.tech>
The url field makes the update command clone the repo. However, the github profile and repo link in the build examples are placeholders, and make the test fail.

Signed-off-by: Victor Moene <victor.moene@northern.tech>
@victormlg victormlg force-pushed the update_module_by_urls branch from b03caa8 to 5e8bc60 Compare June 30, 2025 12:40
@olehermanse olehermanse requested a review from larsewi June 30, 2025 13:40
Signed-off-by: Victor Moene <victor.moene@northern.tech>
Signed-off-by: Victor Moene <victor.moene@northern.tech>
@victormlg victormlg force-pushed the update_module_by_urls branch from 5e8bc60 to 8698f13 Compare June 30, 2025 15:06
@victormlg victormlg requested a review from larsewi June 30, 2025 15:08
@olehermanse olehermanse merged commit 2e7c616 into cfengine:master Jul 1, 2025
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

4 participants