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

skip self links check #258

Merged
merged 3 commits into from
Apr 27, 2022
Merged

skip self links check #258

merged 3 commits into from
Apr 27, 2022

Conversation

jonhealy1
Copy link
Contributor

@jonhealy1 jonhealy1 commented Mar 8, 2022

Related Issue(s):
radiantearth/stac-spec#1173
stac-utils/stac-check#76

Description:
I am removing the warning for no self links.

PR checklist:

  • Code is formatted (run scripts/format).
  • Code lints properly (run scripts/lint).
  • Tests pass (run scripts/test).
  • Documentation has been updated to reflect changes, if applicable.
  • Changes are added to the CHANGELOG.

@gadomski
Copy link
Member

gadomski commented Mar 9, 2022

This feels like something that could be useful if pushed all the way down to stac-check. Two options I see right off the bat:

  • Add a single check_self_links: bool parameter to the checker to enable/disable self link checks
  • Add an ignored_checks List that could be used to disable a variety of checks on the checker

What do you think?

@jonhealy1
Copy link
Contributor Author

This feels like something that could be useful if pushed all the way down to stac-check. Two options I see right off the bat:

  • Add a single check_self_links: bool parameter to the checker to enable/disable self link checks
  • Add an ignored_checks List that could be used to disable a variety of checks on the checker

What do you think?

Both options sound good. Maybe an ignored checks list would best but checks having a boolean field is good too.

@jonhealy1
Copy link
Contributor Author

I think maybe we have to update how the config file is brought in from stac-check - I might just be confused

@jonhealy1
Copy link
Contributor Author

It seems to throw an error on the else statement and then never checks for the config file

        default_config_file = os.getenv("STAC_CHECK_CONFIG")
        if default_config_file:
            with open(default_config_file) as f:
                default_config = yaml.load(f, Loader=yaml.FullLoader)
        else:
            with pkg_resources.resource_stream(__name__, "stac-check.config.yml") as f:
                default_config = yaml.load(f, Loader=yaml.FullLoader)
        if config_file:
            with open(config_file) as f:
                config = yaml.load(f, Loader=yaml.FullLoader)
            default_config.update(config)
            
        return default_config

@jonhealy1
Copy link
Contributor Author

Maybe I wasn't setting the .env properly?

@jonhealy1
Copy link
Contributor Author

It is working now but the file is in an awkward place I think

@gadomski
Copy link
Member

gadomski commented Apr 26, 2022

I've pushed a commit to your branch that works around the issue in a different way -- we fall back on the default stac-check configuration, and simply disable the config value for the self links check. We also add a command line flag that allows the user to pass in their own configuration file, in case they want to re-enable it (or tweak the configuration any other way). What do you think? Feel free to add a review if you have any suggestions/changes.

@jonhealy1
Copy link
Contributor Author

@gadomski I don't see it - maybe it didn't push properly?

@gadomski
Copy link
Member

7b248fc ?

@jonhealy1
Copy link
Contributor Author

Haha sorry I thought you meant push to my branch on stac-check

@jonhealy1
Copy link
Contributor Author

Looks great I think

jonhealy1 and others added 3 commits April 27, 2022 09:24
Allows the user to specify the configuration file from the command line in case
they want to override this behavior.
@gadomski
Copy link
Member

Great thanks! I cleaned up the git history, will merge when CI passes. Thanks for the update!!

@gadomski gadomski merged commit a7fad55 into stac-utils:main Apr 27, 2022
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.

2 participants