Skip to content
Snippets Groups Projects
This project is mirrored from https://github.com/metabase/metabase. Pull mirroring updated .
  1. Jan 10, 2023
  2. Jan 09, 2023
  3. Jan 06, 2023
  4. Jan 05, 2023
    • dpsutton's avatar
      Cache and deps fixes (#27509) · fe512959
      dpsutton authored
      * upgrade clojure cli version
      
      * bump clojure cli in dockerfile
      
      * use expanded cache paths
      
      * list cache contents
      
      * ls repository
      
      * enumerate m2 cache
      
      * remove count of cached dirs
      
      * restore `~` in cache paths
      
      * Revert "remove count of cached dirs"
      
      This reverts commit e768912b31075c7b19b4a3ccdb66c0248ea06c87.
      
      * Revert "Revert "remove count of cached dirs""
      
      This reverts commit 9eeaff2190d78714770d8ec9f64b015c75085876.
      
      * trailing slash seems harmless but unnecessary
      Unverified
      fe512959
    • dpsutton's avatar
      Notify api key not set hints (#27515) · 55e063e5
      dpsutton authored
      * nit alignment
      
      * point to var so changes are picked up
      
      need to refresh this ns after changing the middleware since these add a
      bit more helpful doc strings. But capture previous function value so
      more annoying at repl
      
      * Ensure MB_API_KEY is set for notify endpoint
      
      Previously when the MB_API_KEY was unset:
      
      ```
      ❯ http POST "localhost:3000/api/notify/db/1?scan=schema"
      HTTP/1.1 403 Forbidden
      <other headers removed>
      
      Forbidden
      ```
      
      And now:
      
      ```
      ❯ http POST "localhost:3000/api/notify/db/1?scan=schema"
      HTTP/1.1 403 Forbidden
      <other headers removed>
      
      MB_API_KEY is not set. See https://www.metabase.com/docs/latest/configuring-metabase/environment-variables#mb_api_key for details
      ```
      
      An annoying thing in the tests. We set `"-Dmb.api.key=test-api-key"` in
      the `:test` alias for CI, but not in regular dev mode. So annoyingly we
      either have tests that would fail in the repl in regular dev, or use
      `mt/with-temporary-setting-values [api-key "test-api-key"]` when writing
      tests that only matter for local stuff. c'est la vie
      
      * Translate error message
      
      Using site locale because this in an `+apikey` context, not an `+auth`
      context. The importance of that is that there is no logged in user so no
      way to get a user locale, and it just falls back to the server locale
      anyways. So lets just use that.
      
      * Fix temp settings when they are env-based
      
      An annoying bit here. `api-key` setting in the notify test is set in the
      env in the `:test` alias.
      
      ```clojure
      notify-test=> (mt/with-temporary-setting-values [api-key nil]
                      (mw.auth/api-key))
      "test-api-key" ;; where did the nil go?
      ```
      
      But there are two bugs in `mt/with-temporary-setting-values`:
      
      1. It gets the current env value and then temporarily sets it to the
      current value. Note that `do-with-temp-env-var-value` is called with
      `env-var-value` which is the current value in the environment. All of
      this work for nothing
      
      ```
      -    (if-let [env-var-value (and (not raw-setting?) (#'setting/env-var-value setting-k))]
      -      (do-with-temp-env-var-value setting env-var-value thunk)
      +    (if (and (not raw-setting?) (#'setting/env-var-value setting-k))
      +      (do-with-temp-env-var-value setting-k value thunk)
      ```
      So cannot possibly do what we want.
      
      2. The second bug is that the pathway through
      `mt/with-temporary-setting-values` did not convert `:api-key` to
      `:mb-api-key`. So even if it put our new value in above, it would put
      `:api-key nil` in the env, but when we look for it it would look for
      `:mb-api-key` and we would not find it.
      
      Thus the use of `setting-env-map-name`. Then clean up the few other
      places that did it kinda right, but not necessarily. Seems the "correct"
      way to do this is get the setting name, munge it, prepend with mb. The
      other call sites were using variants of `(keyword (str "mb-" (name
      setting-name)))` which is close but could miss the munging.
      
      * Safely set env/env keys
      
      There are two ways that we put things into the env/env so they appear as
      environmentally set variables.
      
      - explicitly with `mt/with-temp-env-var-value`. This simulates a user
      setting these values and they are specified in the env way:
      mb-email-smtp-port, "MB_DISABLE_SCHEDULER", etc. At the call site you
      are aware that they are env related so get the mb prefix
      - incidentally with `mt/with-temporary-setting-values`. Settings are
      always referred to by their "nice" internal name like `api-key`,
      etc. But if a setting is found to be an env variable, we override it in
      env/env. But we have to make sure the key is set properly with an mb-
      prefix.
      
      Owing to this, we have to conditionally add the mb- prefix if not
      already present.
      
      * cleanup the constants a bit
      
      * Fix some site-url tests
      
      The tests themselves are testing useful stuff: site-url doesn't return a
      broken value if it fails the verification when set as an env
      variable. But the tests themselves were a bit nonsensical in their
      setup:
      
      ```
      @@ -62,21 +62,19 @@
       (deftest site-url-settings-normalize
         (testing "We should normalize `site-url` when set via env var we should still normalize it (#9764)"
           (mt/with-temp-env-var-value [mb-site-url "localhost:3000/"]
      -      (mt/with-temporary-setting-values [site-url nil]
      -        (is (= "localhost:3000/"
      -               (setting/get-value-of-type :string :site-url)))
      -        (is (= "http://localhost:3000"
      -               (public-settings/site-url)))))))
      +      (is (= "localhost:3000/"
      +             (setting/get-value-of-type :string :site-url)))
      +      (is (= "http://localhost:3000"
      +             (public-settings/site-url))))))
      ```
      
      Why would it set an env-variable version [mb-site-url "localhost:3000/"]
      then try to blank it out with `(mt/with-temporary-setting-values
      [site-url nil])`.
      
      Fixing the bug in how we set env var values made this nonsensical stuff
      break.
      
      Now we just have to do the obvious thing: set the env var to a bad
      value, assert that (setting/get-value-of-type :string :site-url) is that
      value, but assert that the _setting_ is a well-formatted value or nil if
      we can't solve it (the "asd_12w31%$;" case).
      
      Setting the env value then setting a temporary value to nil didn't make
      much sense. And the fix made that set the env var to nil.
      
      * fixup last tests
      
      * Don't modify things from `with-temp-env-var-value`
      
      this allows us to set non-mb-prefixed env vars. not sure if necessary
      but let's roll with it.
      
      * fix last of the tests
      Unverified
      55e063e5
    • Bryan Maass's avatar
      Defendpoint autogen dox polish (#27492) · 90e293ad
      Bryan Maass authored
      * fix automatic dox generation for malli+defendpoint
      
      - there isn't a concept of not having an error message for a schema \o/
      - warn but don't break when the schema is invalid
      - polish the descriptions a little "a nullable a vector" -> "a nullable vector"
      
      * gitignore malli types files
      
      * schema -> defendpoint api/setting GET :key
      
      + docs example
      
      * schema -> defendpoint api/action + dox generation
      
      * quickfix + add smaller trivial tests
      
      * add imported clj kondo rules to git
      
      * add with-api-error-message to fix some tests
      
      * use mu/with-api-error-message
      
      * fix tests
      
      * finish defendpoint migration for api/action ns
      
      * do not stringify errors
      
      * fix devtime annoyance with malli-dox generation
      
      * more polish
      
      - add length to numerics
      - respond to review
      
      * unused var linter fix
      
      * fix off by 1 in umd/description
      
      * aligator eats the big number + update action.md
      Unverified
      90e293ad
    • adam-james's avatar
      Disallow axis ranges with the same min/max, which causes divide by zero error (#27518) · 716b87b8
      adam-james authored
      * Disallow axis ranges with the same min/max, which causes divide by zero error
      
      The group-axes private fn is used to try group series axes together if the ranges of the series are 'close enough',
      which is calculated by determining the percentage overlap the ranges share.
      
      As the comparison is made, the first series in the list will always be compared with itself, and should have
      1.0 (100%) overlap, as the ranges are going to be identical. The divide by zero issue arises when this first series,
      and potentially any other series, has a 'range' where the min and max are the same. This happens if the series has the
      same value in every row.
      
      With this change, these ranges are considered invalid, and we avoid the divide by zero.
      
      * Unskip e2e repro
      Unverified
      716b87b8
    • Anton Kulyk's avatar
      Improve sample state fixture (#27399) · 22c19cb6
      Anton Kulyk authored
      * Remove not used `StaticEntitiesProvider`
      
      * Fix typos
      
      * Convert sample database fixture to TypeScript
      
      * Fix `EnhancedState` typing
      
      * Remove global eslint comment
      
      * Fix invalid import
      
      * Fix incorrect type
      Unverified
      22c19cb6
Loading