Skip to content

add exclude snapshot repo option to virtual snapshot#10307

Merged
sameluch merged 3 commits into3.0-devfrom
sammeluch/add-snapshot-exclude-repo
Sep 19, 2024
Merged

add exclude snapshot repo option to virtual snapshot#10307
sameluch merged 3 commits into3.0-devfrom
sammeluch/add-snapshot-exclude-repo

Conversation

@sameluch
Copy link
Copy Markdown
Contributor

Merge Checklist

All boxes should be checked before merging the PR (just tick any boxes which don't apply to this PR)

  • The toolchain has been rebuilt successfully (or no changes were made to it)
  • The toolchain/worker package manifests are up-to-date
  • Any updated packages successfully build (or no packages were changed)
  • Packages depending on static components modified in this PR (Golang, *-static subpackages, etc.) have had their Release tag incremented.
  • Package tests (%check section) have been verified with RUN_CHECK=y for existing SPEC files, or added to new SPEC files
  • All package sources are available
  • cgmanifest files are up-to-date and sorted (./cgmanifest.json, ./toolkit/scripts/toolchain/cgmanifest.json, .github/workflows/cgmanifest.json)
  • LICENSE-MAP files are up-to-date (./LICENSES-AND-NOTICES/SPECS/data/licenses.json, ./LICENSES-AND-NOTICES/SPECS/LICENSES-MAP.md, ./LICENSES-AND-NOTICES/SPECS/LICENSE-EXCEPTIONS.PHOTON)
  • All source files have up-to-date hashes in the *.signatures.json files
  • sudo make go-tidy-all and sudo make go-test-coverage pass
  • Documentation has been updated to match any changes to the build system
  • Ready to merge

Summary

What does the PR accomplish, why was it needed?
After discussing snapshot functionality slightly deeper, the ability to exclude a repo from the snapshot is desired and useful. The patch has added --excludesnapshot option for the command line of tdnf

Change Log
  • Add --excludesnapshot patch to tdnf
Does this affect the toolchain?

YES

Test Methodology
  • Local testing

@sameluch sameluch requested review from a team as code owners August 30, 2024 21:19
@sameluch sameluch force-pushed the sammeluch/add-snapshot-exclude-repo branch from 40c011a to 0c9c773 Compare September 5, 2024 16:29
Comment thread SPECS/tdnf/tdnf.spec Outdated

%changelog
* Thu Aug 29 2024 Sam Meluch <sammeluch@microsoft.com> - 3.5.6-3
- Add virtual repo snapshot exclude
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is there a reason we want a separate argument for the snapshot instead of relying on the existing --disablerepo and --enablerepo? I'm worried that having two may be confusing when using the snapshot.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The reason to add a snapshot exclude option is to allow for a repo containing new packages locally built to still be included when building an image, but not from upstream. This is also additional flexibility within the feature for tdnf, not just applicable to our use case for allowing local RPMs from within the toolkit.

Comment thread SPECS/tdnf/virtual-repo-snapshot.patch Outdated
+{
+ uint32_t dwError = 0;
+ int nIsGlob = 0;
+ if(!pRepos && IsNullOrEmptyString(pszId))
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

&& or || here?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good catch, this is actually copy paste from upstream, we can likely contribute this small fix upstream if not already present

Comment thread SPECS/tdnf/virtual-repo-snapshot.patch Outdated
" [--enableplugin=<plugin_name>]\n"
- " [--snapshottime=<POSIX_time>]\n"
" [--exclude [file1,file2,...]]\n"
+ " [--excludesnapshot=<repoid>]\n"
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

can we rename this to something like --snapshotexclusionrepos so that it sorts with snapshottime. And helps clarify that the snapshot exclusion applies to a repo.

Also...how big of a stretch from here to take a comma separated list of repos (I know we discussed supporting only 1)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Updated to --snapshotexcluderepos to make the names come together.

I also added the ability to specify multiple repos in one go as a comma separated list in line.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

tested by running the following commands to check if the second repo was snapshot when using the updated tdnf:
tdnf install valgrind --snapshottime=1724119509 --snapshotexcluderepos=azurelinux-official-base
tdnf install valgrind --snapshottime=1724119509 --snapshotexcluderepos=azurelinux-official-base,azurelinux-official-base-debug
image

@sameluch sameluch force-pushed the sammeluch/add-snapshot-exclude-repo branch from f94aa40 to 1430837 Compare September 18, 2024 23:27
@sameluch sameluch merged commit 0d3bfba into 3.0-dev Sep 19, 2024
@sameluch sameluch deleted the sammeluch/add-snapshot-exclude-repo branch September 19, 2024 00:41
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.

3 participants