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

publish 0.3.0 #99

Merged
merged 27 commits into from
Aug 21, 2020
Merged

publish 0.3.0 #99

merged 27 commits into from
Aug 21, 2020

Conversation

matthewhanson
Copy link
Member

No description provided.

@@ -10,7 +10,7 @@

logger = logging.getLogger(__name__)

API_URL = os.getenv('STAC_API_URL', 'https://1tqdbvsut9.execute-api.us-west-2.amazonaws.com/v0').rstrip('/') + '/'
API_URL = os.getenv('STAC_API_URL', 'https://earth-search.aws.element84.com/v0').rstrip('/') + '/'
Copy link
Member

Choose a reason for hiding this comment

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

👋 @matthewhanson what do you think about removing any hardcoded URL? you could still use STAC_API_URL environ but I think it will be better if stac-search wasn't tied to any URL.

We (Devseed) will love to fix the API we are hosting but it means first we have to wait for the specification do be complete.

@matthewhanson
Copy link
Member Author

@vincentsarago Sure, I think it's fine to not have any default URL.

Let's see, I'll

  • return error with message that STAC_API_URL needs to be set, or url needs to be provided when creating the Search object
  • add into the README a list of possible compatible URLs for the version (right now just earth-search for this version, and I could also add in info on using cmr-stac).

@vincentsarago
Copy link
Member

return error with message that STAC_API_URL needs to be set, or url needs to be provided when creating the Search object

Here what I had in mind:

  • the Search object should be decoupled from any environment variable
  • it's up to the application (custom script) to pass correct API url to the search Search
  • URL should be an arg (so it errors automatically without)
  • The CLI should have an --url option (which could fall back to STAC_API_URL)

@scottyhq
Copy link

@matthewhanson - could this be merged? Trying to work on intake/intake-stac#49 with sat-stac 0.4.0, but the latest release of sat-search pins - sat-stac >=0.3.0,<0.4

@matthewhanson matthewhanson changed the title publish 0.3.0 publish 0.3.0-rc2 Aug 18, 2020
README.md Outdated
api.us-east-1.amazonaws.com/prod)
--headers HEADERS
JSON Request Headers (file or string) (default: None)
--url URL URL of the API (default: https://earth-search.aws.element84.com/v0/)
Copy link
Member

Choose a reason for hiding this comment

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

there is no more default here!

tutorial-1.ipynb Outdated
@@ -12,7 +12,7 @@
"\n",
"Only the `search` module is in sat-search is used as a library, and it contains a single class, `Search`. The `parser` module is used for creating a CLI parser, and `main` contains the main function used in the CLI.\n",
"\n",
"**API endpoint**: Sat-search uses an endpoint defined by the SATUTILS_API_URL environment variable. This defaults to https://sat-api.developmentseed.org/ but can point to any STAC compatible endpoint."
"**API endpoint**: Sat-search uses an endpoint defined by the STAC_API_URL environment variable. This defaults to https://earth-search.aws.element84.com/v0 but can point to any STAC compatible endpoint."
Copy link
Member

Choose a reason for hiding this comment

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

no more default!

@vincentsarago
Copy link
Member

couple changes needed. Tests are failing because there is no default URL provided for tests.

you can set an env variable here https://github.com/sat-utils/sat-search/blob/develop/.circleci/config.yml#L11 pointing the a viable API.

@matthewhanson
Copy link
Member Author

Thanks @vincentsarago , I paused getting this updated because I realized I also need to fix how pagination is done to get this working with other STAC APIs, and so am updating stac-server and Earthsearch paging since am also doing that incorrectly.

Once I get that deployed I'll come back and get out an rc2 of this today.

@matthewhanson matthewhanson changed the title publish 0.3.0-rc2 publish 0.3.0 Aug 21, 2020
satsearch/cli.py Outdated
@@ -36,7 +38,7 @@ def __init__(self, *args, **kwargs):
self.output_group.add_argument('--print-md', help=h, default=None, nargs='*', dest='printmd')
h = 'Print calendar showing dates'
self.output_group.add_argument('--print-cal', help=h, dest='printcal')
self.output_group.add_argument('--print-assets', help=h, dest='printassets', default=False, action='store_true')
#h = 'Print Item Asset definition from Collections'
Copy link
Member

Choose a reason for hiding this comment

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

?


items = []
found = self.found(headers=headers)
found = 0 #self.found(headers=headers)
Copy link
Member

Choose a reason for hiding this comment

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

what is the comment for?

items += [Item(i) for i in self.query(url=url, headers=headers, **kwargs)['features']]
kwargs['page'] += 1

maxitems = limit #min(found, limit)
Copy link
Member

Choose a reason for hiding this comment

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

same

Search.query(url=os.path.join(API_URL, 'collections/nosuchcollection'))
#def _test_query_bad_url(self):
# with self.assertRaises(SatSearchError):
# Search.query(url=os.path.join(API_URL, 'collections/nosuchcollection'))
Copy link
Member

Choose a reason for hiding this comment

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

why is this removed?

Copy link
Member

@vincentsarago vincentsarago left a comment

Choose a reason for hiding this comment

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

Just a couple notes LGTM!

@matthewhanson matthewhanson merged commit 276a055 into master Aug 21, 2020
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