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

Core as subtree #119

Merged
merged 7 commits into from
May 11, 2021
Merged

Core as subtree #119

merged 7 commits into from
May 11, 2021

Conversation

duckontheweb
Copy link
Contributor

Related Issue(s):
#114

Proposed Changes:

  1. Make stac-spec a git subtree instead of a git submodule

PR Checklist:

  • This PR is made against the dev branch (all proposed changes except releases should be against dev, not master).
  • This PR has no breaking changes.
  • I have added my changes to the CHANGELOG or a CHANGELOG entry is not required.

@duckontheweb
Copy link
Contributor Author

@cholmes @matthewhanson @m-mohr This a demo of what it would look like have stac-spec embedded as a subtree instead of a submodule. A couple of things to note:

  • This is currently using v1.0.0-rc.2 of stac-spec. When we want to update, we will need to use git subtree pull from the new core spec version.
  • Links to core spec docs (e.g. Catalog, Collection) will be to files in this repo and not in radiantearth/stac-spec.
  • I added an item to the PR checklist to help contributors remember not to change anything in the stac-spec directory. If a subtree seems like the right way to go, I'll look into adding a CI check as well.

@duckontheweb
Copy link
Contributor Author

Also, looks like the failing tests are due to invalid links to the stac-spec/extensions folder that no longer exists now that extensions have been moved.

@duckontheweb
Copy link
Contributor Author

I'll fix the broken extension links and add a CI check so you all can review this properly.

@duckontheweb duckontheweb changed the title [WIP] Core as subtree Core as subtree May 4, 2021
@duckontheweb
Copy link
Contributor Author

@cholmes @m-mohr @matthewhanson This is ready for review.

I added an npm command to check for changes to the stac-spec directory against the given revision/branch. It will return a -1 exit code if there have been changes. Also added this as a job in the CircleCI config.

@@ -15,7 +15,8 @@
"check-openapi-item-search": "spectral lint item-search/openapi.yaml --ruleset .circleci/.spectral.yml",
"check-openapi-fragments": "spectral lint fragments/*/openapi.yaml --ruleset .circleci/.spectral-fragments.yml",
"build-openapi": ".circleci/build-openapi.sh",
"publish-openapi": "node .circleci/publish.js"
"publish-openapi": "node .circleci/publish.js",
"check-stac-spec-changes": "git diff --quiet HEAD ${npm_config_compare_to} -- stac-spec"
Copy link
Collaborator

@m-mohr m-mohr May 10, 2021

Choose a reason for hiding this comment

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

This is meant to be used by CI only, right? As I think this would fail on any other system (at least on Win), maybe just use that command directly in the CI without exposing it as npm script?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It can be run locally (I've been using it to test prior to putting in the PR to avoid unexpected CI failures). You just need to pass in the --compare-to option manually when running it locally:

npm run check-stac-spec-changes --compare-to=master

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this would fail on any other system (at least on Win)

You may be right about Windows, I've only tested on Linux and MacOS.

Copy link
Collaborator

@m-mohr m-mohr left a comment

Choose a reason for hiding this comment

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

Not ideal, but probably the best solution that we can get right now. Thanks for doing it. Left one comment, otherwise lgtm.

@duckontheweb
Copy link
Contributor Author

Not ideal, but probably the best solution that we can get right now. Thanks for doing it. Left one comment, otherwise lgtm.

Yeah, I totally agree, this feel really heavy-handed for such a simple problem. If a better solution comes along we should definitely take advantage.

@m-mohr m-mohr merged commit a692ddc into dev May 11, 2021
@m-mohr m-mohr deleted the core-as-subtree branch May 11, 2021 15:52
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