Skip to content
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.

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

config: Return default if sub-config key doesn't exist #354

Merged
merged 4 commits into from
Dec 1, 2024

Conversation

shivaraj-bh
Copy link
Member

@shivaraj-bh shivaraj-bh commented Nov 29, 2024

resolves #353

OmConfigError::UnexpectedAttribute which was moved from QualifiedAttrError::UnexpectedAttribute of the now deleted qualified_attr.rs module seems unnecessary? We could just return the default if the sub-config key doesn’t exist?

The error UnxpectedAttribute is also misleading because we make the user think that we expected a correct one from them, but in reality it is the mistake of whoever is calling OmConfig::get_sub_config_under with incorrect root_key (see the function signature for reference).

@srid
Copy link
Member

srid commented Nov 29, 2024

I found one more bug:

unexpected fragment 'default.omnix' in flake reference

Previous behaviour can be reproduced using (which runs fine):

nix shell nixpkgs#nixVersions.nix_2_18 -c nix --accept-flake-config run github:juspay/omnix/0ed2a389d6b4c8eb78caed778e20e872d2a59973 -- ci run github:juspay/omnix/0ed2a389d6b4c8eb78caed778e20e872d2a59973#default.omnix

New behaviour using this PR:

❯ nix shell nixpkgs#nixVersions.nix_2_18 -c nix --accept-flake-config run github:juspay/omnix/omconfig-regression -- ci run github:juspay/omnix/0ed2a389d6b4c8eb78caed778e20e872d2a59973#default.omnix

👟 Reading om.ci config from flake
❄️  nix --accept-flake-config --version️
❄️  nix --extra-experimental-features nix-command --accept-flake-config show-config --json️
❄️  nix --accept-flake-config flake metadata --json 'github:juspay/omnix/0ed2a389d6b4c8eb78caed778e20e872d2a59973#default.omnix'️
Error: Nix command error: Command error: Process exited unsuccessfully. exit_code=Some(1) stderr=error: unexpected fragment 'default.omnix' in flake reference 'github:juspay/omnix/0ed2a389d6b4c8eb78caed778e20e872d2a59973#default.omnix'


Caused by:
    0: Command error: Process exited unsuccessfully. exit_code=Some(1) stderr=error: unexpected fragment 'default.omnix' in flake reference 'github:juspay/omnix/0ed2a389d6b4c8eb78caed778e20e872d2a59973#default.omnix'

    1: Process exited unsuccessfully. exit_code=Some(1) stderr=error: unexpected fragment 'default.omnix' in flake reference 'github:juspay/omnix/0ed2a389d6b4c8eb78caed778e20e872d2a59973#default.omnix'

Copy link
Member

@srid srid left a comment

Choose a reason for hiding this comment

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

Given the brittle nature of these code paths, could you add tests covering various cases of ci run <ARG>?

https://github.com/juspay/omnix/blob/main/crates/omnix-cli/tests/command/ci.rs

At least, we need cases covering:

@srid srid merged commit df00938 into main Dec 1, 2024
5 checks passed
@srid srid deleted the omconfig-regression branch December 1, 2024 01:51
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.

Config regression? Error: Unexpected attribute
2 participants