Skip to content

Maintenance and fixes#1

Merged
josefigueroa-nedap merged 5 commits into
masterfrom
maintenance-catalog-queries
May 1, 2020
Merged

Maintenance and fixes#1
josefigueroa-nedap merged 5 commits into
masterfrom
maintenance-catalog-queries

Conversation

@josefigueroa-nedap
Copy link
Copy Markdown

@josefigueroa-nedap josefigueroa-nedap commented May 1, 2020

Fixes the catalog-services query, the parameters handling of prepared query operations and updates dependencies.

The catalog-services query returned a hashmap with wrong keys (kebab-cased). The keys in the resulting hashmap are service names. Those service names would change with the kebab-case transformation, becoming names of services that doesn't exists.

I.E. service_api_v1_production -> service-api-v-1-production.

All changes are able to be streamed.

… prepared query operations. Updated dependencies. Added test for the catalog-services query.
@josefigueroa-nedap josefigueroa-nedap requested a review from a team May 1, 2020 10:02
@josefigueroa-nedap josefigueroa-nedap self-assigned this May 1, 2020
Copy link
Copy Markdown

@jwkoelewijn jwkoelewijn left a comment

Choose a reason for hiding this comment

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

The query explain-prepared-query also changes by removing one level in the result hashmap.

Why was this changed? What problem is being solved by this?

What is the reason for this unwrapping? It does change the API quite significantly, so might at least get some explanation for the reason...

Also, this changes (especially the unwrapping of :body) makes our fork incompatible with the upstream version, which might be good to reflect somehow; either in the Readme, or by removing the link to the original project. If we choose to do the latter we might need to at least add an acknowledgement in the readme as well, indicating that this work was heavily influenced by the original project... Especially because this is a "open" fork of the original.

Comment thread README.md Outdated
Comment thread src/consul/core.clj Outdated
([conn {:as params}]
(consul-index conn :get [:catalog :datacenters] {:query-params params})))
(-> (consul-index conn :get [:catalog :datacenters] {:query-params params})
:body)))
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

From the PR description it is not clear why this is done...

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I will extend the explanation.

josefigueroa-nedap and others added 2 commits May 1, 2020 15:51
Co-authored-by: jwkoelewijn <janwillem.koelewijn@nedap.com>
…in-prepared-query operation. Explained why the catalog-services operation was changed.
@josefigueroa-nedap
Copy link
Copy Markdown
Author

I removed the unwrapping to keep consistency with the previous expected result of explain-prepared-query.

@josefigueroa-nedap josefigueroa-nedap merged commit a6de034 into master May 1, 2020
Copy link
Copy Markdown
Member

@thumbnail thumbnail left a comment

Choose a reason for hiding this comment

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

pending review

Comment thread src/consul/core.clj
([conn query-id params]
(->> (consul conn :get [:query (.toString query-id) :execute] :query-params params)
:body
(cske/transform-keys csk/->kebab-case-keyword))))
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

toString instead of str

Comment thread src/consul/core.clj
(-> (consul conn :get [:query (.toString query-id) :explain] {:query-params params})
:body
:Query)))

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Newline police🚨

Comment thread src/consul/core.clj
([conn {:keys [dc] :as params}]
(:body (consul-index conn :get [:catalog :services] {:query-params params}))))
([conn {:as params}]
(shallow-nameify-keys (:body (consul conn :get [:catalog :services] {:query-params params})))))
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Maybe introduce threading here

Comment thread README.md Outdated

### 0.7.4

Fixed `catalog-services` query to provide a hashmap wher the keys are the service name.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
Fixed `catalog-services` query to provide a hashmap wher the keys are the service name.
Fixed `catalog-services` query to provide a map indexed by service name.

also fixes a typo :P

@tcoenraad tcoenraad deleted the maintenance-catalog-queries branch February 4, 2022 13:21
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