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

Decouple changes in providers repo #312

Merged
merged 19 commits into from Jun 25, 2020
Merged

Decouple changes in providers repo #312

merged 19 commits into from Jun 25, 2020

Conversation

shyamd
Copy link
Contributor

@shyamd shyamd commented Jun 11, 2020

Fix #311
This incorporates the providers data as a sub-module to decouple it.

@shyamd shyamd changed the title Fix 311 - Decouple changes in providers repo Decouple changes in providers repo Jun 11, 2020
@shyamd shyamd requested a review from CasperWA June 11, 2020 21:29
@codecov
Copy link

codecov bot commented Jun 12, 2020

Codecov Report

Merging #312 into master will decrease coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #312      +/-   ##
==========================================
- Coverage   90.92%   90.92%   -0.01%     
==========================================
  Files          54       54              
  Lines        2336     2335       -1     
==========================================
- Hits         2124     2123       -1     
  Misses        212      212              
Flag Coverage Δ
#unittests 90.92% <100.00%> (-0.01%) ⬇️
Impacted Files Coverage Δ
optimade/server/data/__init__.py 100.00% <ø> (ø)
optimade/server/routers/utils.py 97.72% <100.00%> (-0.02%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7471190...3d6ac80. Read the comment docs.

Copy link
Member

@CasperWA CasperWA 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, thanks @shyamd !

Only a few questions and comments.

.gitmodules Show resolved Hide resolved
Dockerfile Show resolved Hide resolved
@shyamd shyamd requested a review from ml-evs June 19, 2020 22:16
ml-evs
ml-evs previously approved these changes Jun 24, 2020
Copy link
Member

@ml-evs ml-evs left a comment

Choose a reason for hiding this comment

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

I have one comment about the frequency of checks, related to my question below, but otherwise happy to accept.

Is there a way to only trigger dependabot if the providers.json file itself changes? The providers repo is also used to host some infrastructure (via netlify), and although it doesn't seem to change much, it would be good to not have to review the PR to find out whether the changes are relevant for us.

- package-ecosystem: submodules
directory: "/"
schedule:
interval: weekly
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
interval: weekly
interval: weekly

I reckon we could make this one daily? We're never going to decline it and it would be good for clients that are continuously deploying this package to remain up to date.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good.

@shyamd
Copy link
Contributor Author

shyamd commented Jun 24, 2020

@CasperWA , @ml-evs Shall I take you two off the reviewers list since we will just octopus merge when we get a chance? I'm taking myself off.

@shyamd
Copy link
Contributor Author

shyamd commented Jun 24, 2020

@ml-evs , it looks like there is no way for dependabot to just watch a file in a submodule for now. We could switch this so that we just keep the sub-module tracked with master, but that could also introduce an unforeseen breaking change which I what I'm trying to avoid.

@ml-evs
Copy link
Member

ml-evs commented Jun 24, 2020

@ml-evs , it looks like there is no way for dependabot to just watch a file in a submodule for now. We could switch this so that we just keep the sub-module tracked with master, but that could also introduce an unforeseen breaking change which I what I'm trying to avoid.

Understood, it's fine as is for me.

@shyamd shyamd merged commit d992811 into Materials-Consortia:master Jun 25, 2020
@shyamd shyamd deleted the fix-311-decouple-providers branch June 26, 2020 01:30
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.

Decouple updates in providers repo
3 participants