Skip to content

Protocols command consumes default branch from env #263

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

Merged
merged 3 commits into from
Feb 11, 2025
Merged

Protocols command consumes default branch from env #263

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
cc03854
Get default branch from env
BaptisteGi Feb 6, 2025
d7fbb21
Add comment on all hardcoded main branch to be fixed later
BaptisteGi Feb 6, 2025
37e8568
Add release note
BaptisteGi Feb 6, 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
1 change: 1 addition & 0 deletions changelog/104.fixed.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
- `protocols` CTL command properly gets default branch setting from environment variable
3 changes: 2 additions & 1 deletion infrahub_sdk/ctl/cli_commands.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,7 +129,7 @@
method: str = "run",
debug: bool = False,
_: str = CONFIG_PARAM,
branch: str = typer.Option("main", help="Branch on which to run the script."),
branch: str = typer.Option("main", help="Branch on which to run the script."), # TODO: Replace main by None
Copy link
Contributor

@FragmentedPacket FragmentedPacket Feb 11, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I'm not understanding the change here, should it be setting the default to None?

Copy link
Contributor

Choose a reason for hiding this comment

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

In my opinion throughout infrahubctl the branch parameter should be an optional string defaulting to None and if not specified we should either not specify the branch or use the default branch provided by the SDK which will default to "main", alternatively something else if the SDK is configured that way with some environment variable.

Copy link
Contributor Author

@BaptisteGi BaptisteGi Feb 11, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Yep with the current setup you can't use environment variable INFRAHUB_DEFAULT_BRANCH.

 ➜  export INFRAHUB_DEFAULT_BRANCH="test1"
 ➜  infrahubctl schema load some.yml   
 The schema in Infrahub was is already up to date, no changes were required
 1 schema processed in 0.138 seconds.

=> This loads schema to main branch

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here is the issue to fix all remaining commands #264 I guess it could be breaking someone's workflow ...

Copy link
Contributor

Choose a reason for hiding this comment

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

It might, although hopefully it wouldn't be that impactful as any changes would be done against a non main branch. But worth including in a changelog to indicate this updated behaviour. Not sure if we need all those todo comments when we have #264, but they might be easier to find I guess.

concurrent: int | None = typer.Option(
None,
help="Maximum number of requests to execute at the same time.",
Expand Down Expand Up @@ -383,6 +383,7 @@

else:
client = initialize_client_sync()
branch = branch or client.default_branch

Check warning on line 386 in infrahub_sdk/ctl/cli_commands.py

View check run for this annotation

Codecov / codecov/patch

infrahub_sdk/ctl/cli_commands.py#L386 

Added line #L386 was not covered by tests
schema.update(client.schema.fetch(branch=branch))

code_generator = CodeGenerator(schema=schema)
Expand Down
2 changes: 1 addition & 1 deletion infrahub_sdk/ctl/exporter.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ def dump(
directory: Path = typer.Option(directory_name_with_timestamp, help="Directory path to store export"),
quiet: bool = typer.Option(False, help="No console output"),
_: str = CONFIG_PARAM,
branch: str = typer.Option("main", help="Branch from which to export"),
branch: str = typer.Option("main", help="Branch from which to export"), # TODO: Replace main by None
concurrent: int = typer.Option(
4,
help="Maximum number of requests to execute at the same time.",
Expand Down
2 changes: 1 addition & 1 deletion infrahub_sdk/ctl/importer.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ def load(
),
quiet: bool = typer.Option(False, help="No console output"),
_: str = CONFIG_PARAM,
branch: str = typer.Option("main", help="Branch from which to export"),
branch: str = typer.Option("main", help="Branch from which to export"), # TODO: Replace main by None
concurrent: int | None = typer.Option(
None,
help="Maximum number of requests to execute at the same time.",
Expand Down
2 changes: 1 addition & 1 deletion infrahub_sdk/ctl/menu.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ def callback() -> None:
async def load(
menus: list[Path],
debug: bool = False,
branch: str = typer.Option("main", help="Branch on which to load the menu."),
branch: str = typer.Option("main", help="Branch on which to load the menu."), # TODO: Replace main by None
_: str = CONFIG_PARAM,
) -> None:
"""Load one or multiple menu files into Infrahub."""
Expand Down
2 changes: 1 addition & 1 deletion infrahub_sdk/ctl/object.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ def callback() -> None:
async def load(
paths: list[Path],
debug: bool = False,
branch: str = typer.Option("main", help="Branch on which to load the objects."),
branch: str = typer.Option("main", help="Branch on which to load the objects."), # TODO: Replace main by None
_: str = CONFIG_PARAM,
) -> None:
"""Load one or multiple objects files into Infrahub."""
Expand Down
2 changes: 1 addition & 1 deletion infrahub_sdk/ctl/repository.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ async def add(
commit: str = "",
read_only: bool = False,
debug: bool = False,
branch: str = typer.Option("main", help="Branch on which to add the repository."),
branch: str = typer.Option("main", help="Branch on which to add the repository."), # TODO: Replace main by None
_: str = CONFIG_PARAM,
) -> None:
"""Add a new repository."""
Expand Down
4 changes: 2 additions & 2 deletions infrahub_sdk/ctl/schema.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,7 @@ def get_node(schemas_data: list[dict], schema_index: int, node_index: int) -> di
async def load(
schemas: list[Path],
debug: bool = False,
branch: str = typer.Option("main", help="Branch on which to load the schema."),
branch: str = typer.Option("main", help="Branch on which to load the schema."), # TODO: Replace main by None
wait: int = typer.Option(0, help="Time in seconds to wait until the schema has converged across all workers"),
_: str = CONFIG_PARAM,
) -> None:
Expand Down Expand Up @@ -159,7 +159,7 @@ async def load(
async def check(
schemas: list[Path],
debug: bool = False,
branch: str = typer.Option("main", help="Branch on which to check the schema."),
branch: str = typer.Option("main", help="Branch on which to check the schema."), # TODO: Replace main by None
_: str = CONFIG_PARAM,
) -> None:
"""Check if schema files are valid and what would be the impact of loading them with Infrahub."""
Expand Down