Skip to content

Conversation

@wvandeun
Copy link
Contributor

@wvandeun wvandeun commented Apr 9, 2025

When a generator runs on a task worker, the client that gets passed when we instantiate a generator, does not have the default branch set to the current active branch.

This leads to a situation where the default branch on the store never gets properly set. Due to this objects that get stored end-up in the correct NodeStoreBranch, but when we retrieve them we always try to fetch them from the main NodeStoreBranch.

@wvandeun wvandeun requested review from fatih-acar and ogenstad April 9, 2025 21:37
@codecov
Copy link

codecov bot commented Apr 9, 2025

Codecov Report

Attention: Patch coverage is 0% with 1 line in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
infrahub_sdk/generator.py 0.00% 1 Missing ⚠️
@@            Coverage Diff             @@
##           stable     #350      +/-   ##
==========================================
- Coverage   72.44%   72.43%   -0.01%     
==========================================
  Files          91       91              
  Lines        8169     8170       +1     
  Branches     1572     1572              
==========================================
  Hits         5918     5918              
- Misses       1840     1841       +1     
  Partials      411      411              
Flag Coverage Δ
integration-tests 22.27% <0.00%> (-0.01%) ⬇️
python-3.10 46.52% <0.00%> (-0.04%) ⬇️
python-3.11 46.52% <0.00%> (-0.04%) ⬇️
python-3.12 46.54% <0.00%> (-0.01%) ⬇️
python-3.13 46.52% <0.00%> (-0.04%) ⬇️
python-3.9 44.89% <0.00%> (-0.01%) ⬇️
python-filler-3.12 25.07% <0.00%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
infrahub_sdk/generator.py 35.00% <0.00%> (-0.45%) ⬇️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Contributor

@dgarros dgarros left a comment

Choose a reason for hiding this comment

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

this change is probably alright but it feels like we should also fix the issue in Infrahub and instantiate the SDK with the right default branch

@ogenstad
Copy link
Contributor

this change is probably alright but it feels like we should also fix the issue in Infrahub and instantiate the SDK with the right default branch

I think this part will be refactored when we look at adding support for using this store and converting GraphQL objects into InfrahubNode objects for transforms as well.

To highlight the issue what happens in the generator looks like this:

        self._init_client = client.clone()
        self._init_client.config.default_branch = self._init_client.default_branch = self.branch_name

With this we initialize a new client using the old config and then override the previous default branch so this part works. The problem is that within client.clone() we call the ._initialize() method which in turn configures the store with the default branch which in most cases will be main.

As the SDK client running the generator will have the correct branch for the proposed change it will create objects in the desired branch.

The problem then comes when we start to use the store as the store has a .set() method which will default to node.get_branch() if a branch wasn't specified for the set command. but the .get() won't have a node to refer to so it will use the main branch.

I think this fixes the immediate issue and what we should change moving forward is that client.clone() should take an optional "branch" parameter, I've also been thinking of providing a parameter to indicate if you want a sync or async client back and potentially an option to pre populate the schema (so that the generator client doesn't have to start by downloading the schema.

@wvandeun wvandeun merged commit 29b8a7d into stable Apr 10, 2025
19 checks passed
@wvandeun wvandeun deleted the wvd-20250409-fix-generator-store branch April 10, 2025 12:27
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