dwyl / hits

:chart_with_upwards_trend: General purpose hits (page views) counter
http://hits.dwyl.com
GNU General Public License v2.0
432 stars 63 forks source link

[PR] Adding content negotiation #204

Closed LuchoTurtle closed 1 year ago

LuchoTurtle commented 1 year ago

closes #168

This PR adds content negotiation so users can call Shield.io's endpoint and have customizable badges on their repositories.

LuchoTurtle commented 1 year ago

I'm just writing a small write-up in the README. Similarly to the rest of the README, I don't go into much detail.

LuchoTurtle commented 1 year ago

The tests should all pass and have 100% coverage. I think the CI has an outdated configuration, given the error.

Error: Tried to map a target OS from env. variable 'ImageOS' (got ubuntu22), but failed. If you're using a self-hosted runner, you should set 'env': 'ImageOS': ... to one of the following: ['ubuntu16', 'ubuntu18', 'ubuntu20', 'win16', 'win19']
LuchoTurtle commented 1 year ago

Finished adding the README. It should be good for review. The CI fails but it's setting up the VM. All tests should pass and the package should still have 100% coverage. Perhaps changing CI on a different PR is prudent.

codecov[bot] commented 1 year ago

Codecov Report

Merging #204 (90c2322) into main (9a0e595) will not change coverage. The diff coverage is 100.00%.

@@            Coverage Diff            @@
##              main      #204   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           11        11           
  Lines           78        98   +20     
=========================================
+ Hits            78        98   +20     
Impacted Files Coverage Δ
lib/hits_web/controllers/hit_controller.ex 100.00% <100.00%> (ø)
lib/hits_web/router.ex 100.00% <100.00%> (ø)

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

nelsonic commented 1 year ago

Trying to run this on my localhost getting:

mix c
ERROR! the application :mime has a different value set for key :types during runtime compared to compile time. Since this application environment entry was marked as compile time, this difference can lead to different behaviour than expected:

  * Compile time value was set to: %{}
  * Runtime value was not set

To fix this error, you might:

  * Make the runtime value match the compile time one

  * Recompile your project. If the misconfigured application is a dependency, you may need to run "mix deps.compile mime --force"

  * Alternatively, you can disable this check. If you are using releases, you can set :validate_compile_env to false in your release configuration. If you are using Mix to start your system, you can pass the --no-validate-compile-env flag

11:04:52.188 [error] Task #PID<0.220.0> started from #PID<0.95.0> terminating
** (stop) "aborting boot"
    (elixir 1.14.1) Config.Provider.boot/2
Function: &:erlang.apply/2
    Args: [#Function<1.104735216/1 in Mix.Tasks.Compile.All.load_apps/3>, [mime: "/Users/n/code/hits/_build/test/lib"]]
** (EXIT from #PID<0.95.0>) an exception was raised:
    ** (ErlangError) Erlang error: "aborting boot"
        (elixir 1.14.1) Config.Provider.boot/2
nelsonic commented 1 year ago

Ran:

MIX_ENV=test mix deps.compile mime --force && mix c

Got

==> mime
Compiling 1 file (.ex)
Generated mime app
Generated hits app

11:07:09.164 [info] Migrations already up
.............................
Finished in 0.4 seconds (0.1s async, 0.2s sync)
29 tests, 0 failures

Randomized with seed 698298
----------------
COV    FILE                                        LINES RELEVANT   MISSED
100.0% lib/hits.ex                                   154       17        0
100.0% lib/hits/application.ex                        40        4        0
100.0% lib/hits/hit.ex                                38        5        0
100.0% lib/hits/repo.ex                                5        0        0
100.0% lib/hits/repository.ex                         45        5        0
100.0% lib/hits/user.ex                               40        5        0
100.0% lib/hits/useragent.ex                          44        5        0
100.0% lib/hits_web/channels/hit_channel.ex           30        3        0
100.0% lib/hits_web/controllers/hit_controller.      209       42        0
100.0% lib/hits_web/controllers/page_controller       11        2        0
100.0% lib/hits_web/endpoint.ex                       50        0        0
100.0% lib/hits_web/gettext.ex                        24        0        0
100.0% lib/hits_web/router.ex                         33        5        0
100.0% lib/hits_web/views/error_helpers.ex            45        0        0
100.0% lib/hits_web/views/error_view.ex               16        1        0
100.0% lib/hits_web/views/hit_view.ex                  3        0        0
100.0% lib/hits_web/views/layout_view.ex               3        0        0
100.0% lib/hits_web/views/page_view.ex                 3        0        0
[TOTAL] 100.0%
----------------
Generating report...
Saved to: cover/

So far so good. 👌

nelsonic commented 1 year ago

@LuchoTurtle PR is assigned to me. I'm making changes on localhost please avoid pushing changes while the PR is in-review you are creating merge conflicts.

nelsonic commented 1 year ago
Unpacking objects: 100% (11/11), 1.27 KiB | 118.00 KiB/s, done.
remote: Total 11 (delta 7), reused 11 (delta 7), pack-reused 0
From github.com:dwyl/hits
   43d6cc8..2a23cfc  content-negotiation -> origin/content-negotiation
error: Your local changes to the following files would be overwritten by merge:
    README.md
Please commit your changes or stash them before you merge.
Aborting
nelsonic commented 1 year ago

This wastes my time. 😢

LuchoTurtle commented 1 year ago

Sorry, I saw the review comment you left and I thought you wanted me to change the code. Regardless, if the README was the only thing conflicting, I only deleted three characters from a single line.

nelsonic commented 1 year ago
git pull
Auto-merging README.md
Auto-merging lib/hits_web/controllers/hit_controller.ex
CONFLICT (content): Merge conflict in lib/hits_web/controllers/hit_controller.ex
Automatic merge failed; fix conflicts and then commit the result.
LuchoTurtle commented 1 year ago

Want me to revert my commit?

nelsonic commented 1 year ago

@LuchoTurtle you've added a lot of code to the index/2 function in lib/hits_web/controllers/hit_controller.ex. ⏳ I've removed more than half of it and all tests still pass. ✂️ ✅ The merge conflicts are an epic time-suck. 😢

nelsonic commented 1 year ago

Generally when something is assigned to someone it's "theirs" to work on. That's what allows remote+async work. Otherwise in a larger team it's instant chaos.

nelsonic commented 1 year ago

Deployed branch to Fly.io to test:

==> Monitoring deployment

 1 desired, 1 placed, 1 healthy, 0 unhealthy
--> v67 deployed successfully

Testing ... https://github.com/dwyl/hits/issues/168#issuecomment-1357738958

nelsonic commented 1 year ago

Custom badge

nelsonic commented 1 year ago

Should auto-deploy to Fly.io once build passes on main. 🚀