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

Schematask #1007

Open
wants to merge 18 commits into
base: main
Choose a base branch
from
Open

Schematask #1007

wants to merge 18 commits into from

Conversation

sashadev-sky
Copy link
Member

@sashadev-sky sashadev-sky commented Sep 10, 2019

Fixes #1006
#959 (<=== Add issue number here)
Also #938 but this updates using a rake task which is at least safer than copy paste

Make sure these boxes are checked before your pull request (PR) is ready to be reviewed and merged. Thanks!

  • PR is descriptively titled 📑 and links the original issue above 🔗
  • tests pass -- look for a green checkbox ✔️ a few minutes after opening your PR -- or run tests locally with rake test
  • code is in uniquely-named feature branch and has no merge conflicts 📁
  • screenshots/GIFs are attached 📎 in case of UI updation
  • ask @publiclab/mapknitter-reviewers for help, in a comment below

We're happy to help you get this ready -- don't be afraid to ask for help, and don't be discouraged if your tests fail at first!

If tests do fail, click on the red X to learn why by reading the logs.

Please be sure you've reviewed our contribution guidelines at https://publiclab.org/contributing-to-public-lab-software

Thanks!

@codeclimate
Copy link

codeclimate bot commented Sep 10, 2019

Code Climate has analyzed commit 5b1f882 and detected 0 issues on this pull request.

View more on Code Climate.

@codecov
Copy link

codecov bot commented Sep 10, 2019

Codecov Report

Merging #1007 into main will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff          @@
##            main   #1007   +/-   ##
=====================================
  Coverage   73.7%   73.7%           
=====================================
  Files         40      40           
  Lines       1388    1388           
=====================================
  Hits        1023    1023           
  Misses       365     365

@sashadev-sky
Copy link
Member Author

@jywarren like mentioned before, I don't think we should keep this file as there are a lot of opportunities for human error when the schema.rb is being copy-pasted and we don't always remember to update it. take #959 for example.

  1. I would delete this file in this PR instead, but if we are going to keep it we should create a way to maintain it.

  2. This updates the schema.rb.example, which is currently outdated, and solves the human error issue by making the updating with a rake task. $ rake copy_schema

If we decided to move forward with 2 instead of 1, we should merge this so they are up to date and i will follow up with a PR to make the rake task run whenever the original schema.rb is updated.

@sashadev-sky
Copy link
Member Author

not sure why the exporter test is failing now, if the last merged PR into main was mine and it wasn't failing there...

@alaxalves I think you've manaegd to fix this before?

 FAIL["test_should_export_warpable_using_isolated_exporter_lib", #<Minitest::Reporters::Suite:0x00007f96dec02598 @name="ExporterTest">, 30.35520500014536]
 test_should_export_warpable_using_isolated_exporter_lib#ExporterTest (30.36s)
        Expected false to be truthy.
        test/models/exporter_test.rb:38:in `block in <class:ExporterTest>'

@jywarren

@alaxalves
Copy link
Member

not sure why the exporter test is failing now, if the last merged PR into main was mine and it wasn't failing there...

@alaxalves I think you've manaegd to fix this before?

 FAIL["test_should_export_warpable_using_isolated_exporter_lib", #<Minitest::Reporters::Suite:0x00007f96dec02598 @name="ExporterTest">, 30.35520500014536]
 test_should_export_warpable_using_isolated_exporter_lib#ExporterTest (30.36s)
        Expected false to be truthy.
        test/models/exporter_test.rb:38:in `block in <class:ExporterTest>'

@jywarren

It makes no sense this test to be breaking. I have no idea 😄 😄 😄

@sashadev-sky
Copy link
Member Author

not sure why the exporter test is failing now, if the last merged PR into main was mine and it wasn't failing there...
@alaxalves I think you've manaegd to fix this before?

 FAIL["test_should_export_warpable_using_isolated_exporter_lib", #<Minitest::Reporters::Suite:0x00007f96dec02598 @name="ExporterTest">, 30.35520500014536]
 test_should_export_warpable_using_isolated_exporter_lib#ExporterTest (30.36s)
        Expected false to be truthy.
        test/models/exporter_test.rb:38:in `block in <class:ExporterTest>'

@jywarren

It makes no sense this test to be breaking. I have no idea 😄 😄 😄

It didn't fail on another PR I just pushed #1011 . But is consistently failing here locally too 🤔

@jywarren
Copy link
Member

Very strange re: exporter test... 😕

As to schema.rb, in plots2 we have for a long time not tracked schema.rb but provided schema.rb.example, which in production on publiclab.org means that we don't get schema.rb conflicts when running migrations. But that, when setting it up in development locally, you can manually copy the schema.rb.example file to schema.rb to get the app running. It's been a pretty stable and reliable system, if you'd like we can reproduce it here?

In fact, we even have a Dangerfile line to warn people to keep schema.rb.example up to date:

https://github.com/publiclab/plots2/blob/19916745ce9f361975cdc769537f8ccf72317766/Dangerfile#L15-L17

What do you think?

@sashadev-sky
Copy link
Member Author

sashadev-sky commented Sep 12, 2019

Yeah sounds good! Rails docs say to resolve mergee conflicts just run $ rake db migrate: https://edgeguides.rubyonrails.org/active_record_migrations.html#schema-dumps-and-source-control so that is another option.

How would you like to proceed here? @jywarren

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Schema.rb
3 participants