Skip to content

Apply migrations based on migrator filename #2369

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.

Sign up for GitHub

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

Draft
h-vetinari wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

Apply migrations based on migrator filename #2369

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7fe8dc0
switch migration application to be determined by (base) filename, not…
h-vetinari Aug 22, 2025
8ad787f
remove timestamp from migration logic and get_migrations_in_dir
h-vetinari Aug 22, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 7 additions & 12 deletions conda_smithy/configure_feedstock.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2694,15 +2694,13 @@ def make_jinja_env(feedstock_directory):
def get_migrations_in_dir(migrations_root):
"""
Given a directory, return the migrations as a mapping
from the (filename, timestamp) to (full_path, migration_number, use_local)
from the filename to a tuple of (full_path, migration_number, use_local)
"""
res = {}
for full_path in glob.glob(os.path.join(migrations_root, "*.yaml")):
with open(full_path, encoding="utf-8") as f:
contents = f.read()
migration_yaml = yaml.load(contents, Loader=yaml.loader.BaseLoader) or {}
# Use a object as timestamp to not delete it
ts = migration_yaml.get("migrator_ts", object())
migration_number = migration_yaml.get("__migrator", {}).get(
"migration_number", 1
)
Expand All @@ -2711,7 +2709,7 @@ def get_migrations_in_dir(migrations_root):
== "true"
)
fn = os.path.basename(full_path)
res[(fn, ts)] = (full_path, migration_number, use_local)
res[fn] = (full_path, migration_number, use_local)
return res


Expand Down Expand Up @@ -2754,14 +2752,12 @@ def set_migration_fns(forge_dir, forge_config):
migrations_in_cfp = get_migrations_in_dir(cfp_migrations_dir)

result = []
for (fn, ts), (full_path, num, use_local) in migrations_in_feedstock.items():
if use_local or not isinstance(ts, (int, str, float)):
# This file has a setting to use the file in the feedstock
# or doesn't have a timestamp. Use it as it is.
for fn, (full_path, num, use_local) in migrations_in_feedstock.items():
if use_local:
result.append(full_path)
elif (fn, ts) in migrations_in_cfp:
elif fn in migrations_in_cfp:
# Use the one from cfp if migration_numbers match
new_full_path, new_num, _ = migrations_in_cfp[(fn, ts)]
new_full_path, new_num, _ = migrations_in_cfp[fn]
if num == new_num:
logger.info(
"%s from feedstock is ignored and upstream version is used",
Expand All @@ -2773,9 +2769,8 @@ def set_migration_fns(forge_dir, forge_config):
else:
# Delete this as this migration is over.
logger.info(
"%s with timestamp %s does not exist in global pinning (anymore), removing it.",
"%s does not exist in global pinning (anymore), removing it.",
fn,
ts,
)
logger.info(
"If it should be applied nevertheless, check that migrator_ts/migration_number match the ones in "
Expand Down
23 changes: 23 additions & 0 deletions news/2368-don't-use-timestamps-to-determine-migration-application.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
**Added:**

* <news item>

**Changed:**

* Avoid using migration timestamps to determine whether migrations should be applied. The logic for ``use_local`` and ``migration_number`` remains unchanged, but migrations are now uniformly applied based on the name of the migrator file, and timestamps are only used to order the application of migrators (#2368).

Comment on lines +7 to +8

@isuruf isuruf Aug 22, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Let's use both the filename and the timestamp. It's hard to keep track of which filenames were used before, that's why the timestamp was used as a unique identifier.

@h-vetinari h-vetinari Aug 22, 2025

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Can you explain how you envision this? To me it still sounds like an inconsistent (or at least very hard-to-follow) mental model. What I'd like is a process that's as simple as:

From what I can tell, we'd never need to care about filenames which occurred in the past, only those that are directly present in the current pinning.

@isuruf isuruf Aug 22, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

we'd never need to care about filenames which occurred in the past

Let's say we had a migrator foo.yaml applied in the past and still present in the feedstock. This migrator was closed and deleted in conda-forge-pinning-feedstock.

Later, another migrator with the same name foo.yaml is applied. Then conda-smithy will consider that old foo.yaml still present in the feedstock is the same as the new foo.yaml and use the new migrator as that file is newer and has updates.

@h-vetinari h-vetinari Aug 22, 2025 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

But isn't that how it should work? The foo.yaml in the global pinning takes precedence, unless overridden by use_local: true.

If the feedstock had been rerendered between the deletion of the old foo.yaml and the creation of the new foo.yaml (both from the POV of the global pinning), the old one would have been deleted in the feedstock anyway. So the fact that it gets replaced with the new one (if it exists) to me sounds like a feature, not a bug

@isuruf isuruf Aug 22, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It's a feature, yes, but it also means that we have to keep track of what was used before to not accidentally use this feature. My example was about two completely different migrators with the same name foo.yaml and they should not override each other.

@h-vetinari h-vetinari Aug 22, 2025

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

The REST API will find up to 4,000 repositories that match your filters and return results from those repositories.

The check would fail on a single hit in exactly the same way as for 10'000 hits. The upper limit is irrelevant for the check I have in mind

@beckermr beckermr Aug 22, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I am almost 100% sure the text implies github won't search all of our repositories. It says

To keep the REST API fast for everyone, we limit the number of repositories a query will search through.

They are not limiting the results, but instead the number of repos searched.

@h-vetinari h-vetinari Aug 22, 2025

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I think your interpretation is incorrect. The limitation is not about searching (which is based on a computed index), but about how many results can (reasonably) be fetched.

@beckermr beckermr Aug 22, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It literally says "repositories a query will search through." I don't know how else to parse that very explicit statement about what is limited since it literally says they limit the number of repositories they search.

@h-vetinari h-vetinari Aug 22, 2025 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

The way I parse this is that the first part is colloquial, and the second part actually reveals the limitation ("finding", not "searching"; that distinction is irrelevant in common parlance, but not here). That argument is bolstered by the fact that their search API even provides an explicit value for whether the results are complete

Got {
  "total_count": 0,
  "incomplete_results": false,      # <----
  "items": [
  ]
}

In your interpretation, "incomplete_results" would always have to be true in orgs with more than 4000 repos. This is not what's observed in practice.

Finally, my interpretation is consistent with the observation that searching a pre-computed index does not provide a way to tell the query "please stop at X repos" (because the index itself has no notion of individual repos, just an amalgamation of their contents), it only provides a way to limit how many results are returned.

**Deprecated:**

* <news item>

**Removed:**

* <news item>

**Fixed:**

* <news item>

**Security:**

* <news item>