Connections #15939
Connections #15939
Conversation
As per review comments. The default environment in Rails is development, so the fallback is better removed. Also, better use Rails `Rails.env` instead of environment variables for checks (at least cleaner and nicer).
Prevent rspec from being executed in any env other than test
* add git tags and docker labels
fix substitutions error
There was a problem hiding this comment.
Apart from what Rubocop is claiming, I have a minor question about if we need to define the version of the google's gems in gemfiles or not. Left some comments, nothing blocking.
Note: rubocop's comments make hard to read the code of the PR. Didn't find how to hide them.
LGTM
| @@ -11,6 +11,7 @@ sudo make install | |||
|
|
|||
| ### Features | |||
There was a problem hiding this comment.
We will need an entry with a description of what's implemented in this PR, won't we?
|
|
||
| def destroy | ||
| @connection_manager.delete_connection(params[:id]) | ||
| head :ok |
There was a problem hiding this comment.
Not familiar with this line, I guess it responds with 200/4 Ok, right?
| service = params[:service] | ||
| connection = @connection_manager.find_oauth_connection(service) | ||
| # shouldn't it return a presented connection? and raise an exception if not found? | ||
| if connection |
There was a problem hiding this comment.
Shouldn't we use methods such as .present?
There was a problem hiding this comment.
Yes, I think's preferable for clarity/style, even though here connection is either a Connection or nil, so it shouldn't make any difference (present? takes into account other values that are not falsy but are empty like [] or blank like '')
| connection = @connection_manager.fetch_connection(params[:id]) | ||
| @connection_manager.check(connection) | ||
| connector = Carto::Connector.new(parameters: {}, connection: connection, user: current_user, logger: nil) | ||
| render_jsonp({ connected: @connection_manager.check(connection) }, 200) |
There was a problem hiding this comment.
We are calling @connection_manager.check(connection) twice for the controller method.
| render_jsonp({ connected: @connection_manager.check(connection) }, 200) | ||
| end | ||
|
|
||
| def projects |
There was a problem hiding this comment.
Question: I guess only the BigQuery connector has the concept of projects, no?.
| end | ||
|
|
||
| def rescue_from_invalid_connection(exception) | ||
| code = exception.message =~ /Access Denied/im ? 401 : 422 |
There was a problem hiding this comment.
403 fits better for an Access denied errors.
|
|
||
| # Unify connectors of dual type | ||
| oauth_connectors.each do |oauth_connector| | ||
| db_connector = db_connectors.find { |c| c[:connector] == oauth_connector[:connector] } |
There was a problem hiding this comment.
db_connectors might not be initialized when not types.include?(Carto::Connection::TYPE_DB_CONNECTOR). no?
There was a problem hiding this comment.
it's initialized to [] at the top, so no problem
….vue Co-authored-by: Raúl Yeguas <neokore@gmail.com>
…search' of https://github.com/CartoDB/cartodb into feature/ch124440/indexing-do-catalog-in-google-dataset-search
…atalog-in-google-dataset-search Added JSON-LD in Data Catalog
![hound[bot]](https://avatars.githubusercontent.com/in/3598?s=60&v=4){kind=small}
| 'spatialCoverage': this.dataset.country_name, | ||
| 'variableMeasured': keyVariables, | ||
| 'version': this.dataset.version, | ||
| 'url': `https://carto.com${this.$router.resolve({ name: 'catalog-dataset-summary', params: {...this.$route.params} }).href}` |
There was a problem hiding this comment.
A space is required after '{' object-curly-spacing
A space is required before '}' object-curly-spacing
See https://app.clubhouse.io/cartoteam/story/117351 & https://app.clubhouse.io/cartoteam/story/117356
Experimental implementation of connections.
PENDING: (delayed decisions)