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

simplified CI setup #163

Merged
merged 3 commits into from
Sep 4, 2019
Merged

simplified CI setup #163

merged 3 commits into from
Sep 4, 2019

Conversation

jameslamb
Copy link
Collaborator

Looking at this project for the first time in a bit while working on #161 , I found that there are some places where can reduce duplication. In this PR, I want to propose some reorganization that will removed opportunities for error and make the exact amount of complexity (e.g. all the ES7-specific exceptions in test setup) more obvious.

after_success:
- Rscript -e 'install.packages("covr"); Sys.setenv(NOT_CRAN = "true"); covr::codecov("r-pkg/")'
- .ci/report_to_covr.sh
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe I'm misunderstanding the diff, but is there a reason you're only running the covr report on 6.2.4? Is it because you only want to run it once?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah it only needs to run once. So right now we're using testthat to run the tests, and then just reporting to codecov (an external service) one time to power the CI checks around coverage and the badge in the README.

Eventually I'd like to just use covr to run tests (like you and I have done in other projects) but didn't want to pull that into this PR.

.ci/install.sh Outdated

if [[ "$TASK" == "pypkg" ]]; then
PY_PACKAGE_DIR=$(pwd)/py-pkg
pip install ${R_PACKAGE_DIR}
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should be PY_PACKAGE_DIR

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

oh good catch

Copy link
Collaborator

@austin3dickey austin3dickey left a comment

Choose a reason for hiding this comment

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

Looks great! I saw that 2 R builds failed but I suspect it was transient?

@jameslamb
Copy link
Collaborator Author

Looks great! I saw that 2 R builds failed but I suspect it was transient?

I'm not sure if it's transient. Lemme try pushing the other changes I have an we'll see. If 6.1.x and 6.2.x fail for BOTH R and Python, we'll know I got the logic for pulling and starting up ES wrong.


"6.2.4")
export ES_VERSION=6.2.4;
export MAPPING_FILE=${ES5_MAPPING_FILE};
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ah here's the error (probably?), I used the wrong mapping file.

Python tests didn't fail because there isn't a get_fields() in the Python package yet. Created #164 to document it.

@codecov-io
Copy link

Codecov Report

Merging #163 into master will decrease coverage by 2.33%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #163      +/-   ##
=========================================
- Coverage   95.14%   92.8%   -2.34%     
=========================================
  Files           8       8              
  Lines         556     556              
=========================================
- Hits          529     516      -13     
- Misses         27      40      +13
Impacted Files Coverage Δ
R/get_fields.R 92.2% <0%> (-7.8%) ⬇️
R/es_search.R 88.12% <0%> (-2.69%) ⬇️

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 118d2bb...12744cc. Read the comment docs.

@jameslamb
Copy link
Collaborator Author

Ok this is passing! @austin3dickey whenever you have time, could you take one more look?

Copy link
Collaborator

@austin3dickey austin3dickey left a comment

Choose a reason for hiding this comment

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

Looks great!

@jameslamb jameslamb merged commit 4a6b39b into uptake:master Sep 4, 2019
@jameslamb jameslamb deleted the simplify_ci branch June 1, 2021 04:07
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