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

Update dsra psra oct2021 (opendrr-api) #154

Merged
merged 30 commits into from
Jan 21, 2022
Merged

Conversation

wkhchow
Copy link
Contributor

@wkhchow wkhchow commented Jan 18, 2022

  • PR along with model-factory Update dsra psra oct2021 (model-factory) model-factory#96
  • updates to support updated DSRA/PSRA requirements
  • bug fixes
  • update latest docker/ES version
  • current PT_LIST in add_data.sh is missing BC, need to add back once BC is ready
  • current canada-srm2 is running off a branch and not master, and may need to remove reference if branch is merged to master

@wkhchow wkhchow added this to the Sprint 50 milestone Jan 18, 2022
@wkhchow wkhchow changed the title Update dsra psra oct2021 Update dsra psra oct2021 (opendrr-api) Jan 18, 2022
@jvanulde jvanulde added Bug Something isn't working Enhancement New feature or request and removed Bug Something isn't working Enhancement New feature or request labels Jan 18, 2022
@jvanulde jvanulde self-requested a review January 19, 2022 18:14
@@ -6,7 +6,7 @@ volumes:
services:

kibana-opendrr:
image: kibana:7.12.0
image: docker.elastic.co/kibana/kibana:7.16.1
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@@ -385,7 +424,7 @@ read_github_token() {
# from the OpenDRR/model-factory repository
get_model_factory_scripts() {
# TODO: Make this more robust
RUN git clone https://github.com/OpenDRR/model-factory.git --depth 1 || (cd model-factory ; RUN git pull)
RUN git clone https://github.com/OpenDRR/model-factory.git --branch update_dsra_psra_oct2021 --depth 1 || (cd model-factory ; RUN git pull)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we move the dependencies to release assets to avoid a clone?

Copy link
Member

Choose a reason for hiding this comment

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

Can we move the dependencies to release assets to avoid a clone?

@jvanulde Intriguing idea! (I've never thought of this before, at least not for model-factory, as I was only thinking of a way out of Git LFS and model-factory doesn't use any and escaped my mind)

But yeah! This would be one way to ensure the versions are consistent between model-factory and opendrr-api for releases, and we do get the "Source code (tar.gz)" release assets for free, automatically generated by GitHub whenever version is tagged and/or released, e.g. https://github.com/OpenDRR/model-factory/archive/refs/tags/v1.2.0.tar.gz

For development (and testing), I guess we could keep the alternative of using git clone to fetch from master or topic branch like update_dsra_psra_oct2021, though apparently we could just fetch these as tarballs too unless we need the git history:

Let's continue the discussion in a new issue! I have just copied the above to:

Copy link
Contributor

Choose a reason for hiding this comment

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

Agree, we should pull from a release. This branch of model-factory has been merged so we are free to revert back to pulling from master

Copy link
Contributor

@jvanulde jvanulde left a comment

Choose a reason for hiding this comment

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

Looks good.

anthonyfok and others added 3 commits January 20, 2022 18:37
This reverts commit 4548803
to avoid merge conflict.  To be applied again at the end of the
commit series in pull request #154
This is similar to the reverted commit 4548803
except unrelated changes that fix Flake8 warnings are no longer
included, to be dealt with in a future pull request.
Copy link
Member

@anthonyfok anthonyfok left a comment

Choose a reason for hiding this comment

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

Great work! Looks good to me! :-)

There is a merge conflict because we were making changes to docker-compose.yml and
python/dsra_postgres2es.py in both master and update_dsra_psra_oct2021 branches. I did an interactive rebase locally (git rebase -i master etc.) and fixed the merge conflict esp. in python/dsra_postgres2es.py by reverting commit 4548803 (that changed more than what the commit log said, my bad) in commit 1e33297, and re-applying it in commit 1e33297.

Since GitHub won't let us merge the original update_dsra_psra_oct2021 branch any other way, I'll proceed to force-push the rebased branch while backing up the original branch as update_dsra_psra_oct2021_ORIGINAL.

@anthonyfok anthonyfok force-pushed the update_dsra_psra_oct2021 branch from 7588ee2 to 1e33297 Compare January 21, 2022 19:28
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Lintly has detected code quality issues in this pull request.

Many of these were previously fixed in commit 4548803 but was reverted
during conflict resolution.
@anthonyfok anthonyfok merged commit 468a91d into master Jan 21, 2022
Copy link
Contributor

@drotheram drotheram left a comment

Choose a reason for hiding this comment

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

approved

@drotheram drotheram linked an issue Feb 5, 2022 that may be closed by this pull request
@jvanulde jvanulde linked an issue Feb 11, 2022 that may be closed by this pull request
@wkhchow wkhchow deleted the update_dsra_psra_oct2021 branch April 4, 2022 18:35
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.

Load dsra_shakemap_scenario_extents into ES index Shakemap datasets don't have geometry via pygeoapi
4 participants