You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2021/04/28 14:36:53 UTC
[GitHub] [airflow] pateash opened a new pull request #15574: #12401 - Duplicating connections from UI
pateash opened a new pull request #15574:
URL: https://github.com/apache/airflow/pull/15574
closes: #12401
---
**Description**
* This change allows users to duplicate connections via UI.
* user can also create duplicates of multiple connections.
* A descriptive flash message will be shown regarding any issue in any of the connection duplication.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on a change in pull request #15574: #12401 - Duplicating connections from UI
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #15574:
URL: https://github.com/apache/airflow/pull/15574#discussion_r629885608
##########
File path: tests/www/views/test_views_connection.py
##########
@@ -54,3 +54,46 @@ def test_prefill_form_null_extra():
cmv = ConnectionModelView()
cmv.prefill_form(form=mock_form, pk=1)
+
+
+def test_duplicate_connection(admin_client):
+ """Test Duplicate multiple connection with suffix"""
+ conn1 = Connection(
+ conn_id='test_duplicate_gcp_connection',
+ conn_type='Google Cloud',
+ description='Google Cloud Connection',
+ )
+ conn2 = Connection(
+ conn_id='test_duplicate_mysql_connection',
+ conn_type='FTP',
+ description='MongoDB2',
+ host='localhost',
+ schema='airflow',
+ port=3306,
+ )
+ conn3 = Connection(
+ conn_id='test_duplicate_postgres_connection_copy1',
+ conn_type='FTP',
+ description='Postgres',
+ host='localhost',
+ schema='airflow',
+ port=3306,
+ )
+ with create_session() as session:
+ session.query(Connection).delete()
+ session.add_all([conn1, conn2, conn3])
+ session.commit()
Review comment:
This part above should be a fixture since it’s test setup, not the actual test.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] github-actions[bot] commented on pull request #15574: #12401 - Duplicating connections from UI
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #15574:
URL: https://github.com/apache/airflow/pull/15574#issuecomment-832932577
[The Workflow run](https://github.com/apache/airflow/actions/runs/814469728) is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on a change in pull request #15574: #12401 - Duplicating connections from UI
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #15574:
URL: https://github.com/apache/airflow/pull/15574#discussion_r625793649
##########
File path: airflow/www/views.py
##########
@@ -3012,6 +3015,57 @@ def action_muldelete(self, items):
self.update_redirect()
return redirect(self.get_redirect())
+ @action(
+ 'mulduplicate',
+ 'Duplicate',
+ 'Are you sure you want to duplicate the selected connections?',
+ single=False,
+ )
+ @provide_session
+ @auth.has_access(
+ [
+ (permissions.ACTION_CAN_CREATE, permissions.RESOURCE_CONNECTION),
+ ]
+ )
+ def action_mulduplicate(self, connections, session=None):
+ """Duplicate Multiple connections"""
+ for selected_conn in connections:
+ new_conn_id = selected_conn.conn_id
+ matches = re.findall(r"_Copy\((\d+)\)", selected_conn.conn_id)
Review comment:
Is it intended to find `_CopyX` *anywhere*? Since the logic below only always adds a suffix, I would expect this to only check for a suffix as well.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] pateash commented on pull request #15574: #12401 - Duplicating connections from UI
Posted by GitBox <gi...@apache.org>.
pateash commented on pull request #15574:
URL: https://github.com/apache/airflow/pull/15574#issuecomment-832567983
> As others have said, we should have it as `_copy` not `_Copy`.
>
> Additionally, it should be `copy1` not `copy(1)` so it can be set via env vars if _has_ to. (You can't put `()` in env var names, so `AIRFLOW_CONN_FOO_COPY(1)` would not be possible
Thanks All for valuable comments,
As per suggestions, I have used **_copyN** Syntax for duplication,
also test has been added.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] pateash commented on a change in pull request #15574: #12401 - Duplicating connections from UI
Posted by GitBox <gi...@apache.org>.
pateash commented on a change in pull request #15574:
URL: https://github.com/apache/airflow/pull/15574#discussion_r625833590
##########
File path: tests/www/test_views.py
##########
@@ -561,15 +626,15 @@ def test_health(self):
BaseJob.job_type == 'SchedulerJob',
BaseJob.state == 'running',
BaseJob.latest_heartbeat == last_scheduler_heartbeat_for_testing_1,
- ).delete()
+ ).delete()
Review comment:
sorry @ashb, Looks like my IDE does that automatically.
I will fix these.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] pateash edited a comment on pull request #15574: Add means to Duplicate connections from UI
Posted by GitBox <gi...@apache.org>.
pateash edited a comment on pull request #15574:
URL: https://github.com/apache/airflow/pull/15574#issuecomment-912976507
@jyotsa09
this is expected behaviour, you should not have any connection with _copyN in the end,
it's just a way to create duplicates, after duplication you are supposed to rename the connection.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #15574: #12401 - Duplicating connections from UI
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #15574:
URL: https://github.com/apache/airflow/pull/15574#issuecomment-852960157
Can you please rebase and solve conflicts @pateash ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] pateash commented on a change in pull request #15574: Add means to Duplicate connections from UI
Posted by GitBox <gi...@apache.org>.
pateash commented on a change in pull request #15574:
URL: https://github.com/apache/airflow/pull/15574#discussion_r650390875
##########
File path: airflow/www/views.py
##########
@@ -3113,6 +3116,55 @@ def action_muldelete(self, items):
self.update_redirect()
return redirect(self.get_redirect())
+ @action(
+ 'mulduplicate',
+ 'Duplicate',
+ 'Are you sure you want to duplicate the selected connections?',
+ single=False,
+ )
+ @provide_session
+ @auth.has_access(
+ [
+ (permissions.ACTION_CAN_CREATE, permissions.RESOURCE_CONNECTION),
Review comment:
sure make sense.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] pateash commented on a change in pull request #15574: #12401 - Duplicating connections from UI
Posted by GitBox <gi...@apache.org>.
pateash commented on a change in pull request #15574:
URL: https://github.com/apache/airflow/pull/15574#discussion_r626793900
##########
File path: airflow/www/views.py
##########
@@ -3012,6 +3015,56 @@ def action_muldelete(self, items):
self.update_redirect()
return redirect(self.get_redirect())
+ @action(
+ 'mulduplicate',
+ 'Duplicate',
+ 'Are you sure you want to duplicate the selected connections?',
+ single=False,
+ )
+ @provide_session
+ @auth.has_access(
+ [
+ (permissions.ACTION_CAN_CREATE, permissions.RESOURCE_CONNECTION),
+ ]
+ )
+ def action_mulduplicate(self, connections, session=None):
+ """Duplicate Multiple connections"""
+ for selected_conn in connections:
+ new_conn_id = selected_conn.conn_id
+ matches = re.search(r"_copy(\d+)$", selected_conn.conn_id)
+ if matches:
+ new_conn_id = selected_conn.conn_id.replace(
+ f'_copy{matches.group(1)}', f'_copy{int(matches.group(1)) + 1}'
+ )
Review comment:
@uranusjr,
it won't fail in any cases as we are only matching the last occurence in the regex.
**r"_copy(\d+)$"**
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on a change in pull request #15574: #12401 - Duplicating connections from UI
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #15574:
URL: https://github.com/apache/airflow/pull/15574#discussion_r626773280
##########
File path: airflow/www/views.py
##########
@@ -3012,6 +3015,56 @@ def action_muldelete(self, items):
self.update_redirect()
return redirect(self.get_redirect())
+ @action(
+ 'mulduplicate',
+ 'Duplicate',
+ 'Are you sure you want to duplicate the selected connections?',
+ single=False,
+ )
+ @provide_session
+ @auth.has_access(
+ [
+ (permissions.ACTION_CAN_CREATE, permissions.RESOURCE_CONNECTION),
+ ]
+ )
+ def action_mulduplicate(self, connections, session=None):
+ """Duplicate Multiple connections"""
+ for selected_conn in connections:
+ new_conn_id = selected_conn.conn_id
+ matches = re.search(r"_copy(\d+)$", selected_conn.conn_id)
+ if matches:
+ new_conn_id = selected_conn.conn_id.replace(
+ f'_copy{matches.group(1)}', f'_copy{int(matches.group(1)) + 1}'
+ )
Review comment:
```suggestion
conn_id_prefix = selected_conn.conn_id[:matches.start()]
new_conn_id = f"{conn_id_prefix}_copy{int(matches.group(1)) + 1}"
```
The current implementation would fail in edge cases like `conn_id_copy1_copy1` (incorrectly replacing the first `copy1`).
Also the variable should probably be named `match` (singular) instead.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] pateash commented on a change in pull request #15574: #12401 - Duplicating connections from UI
Posted by GitBox <gi...@apache.org>.
pateash commented on a change in pull request #15574:
URL: https://github.com/apache/airflow/pull/15574#discussion_r626438955
##########
File path: airflow/www/views.py
##########
@@ -3012,6 +3015,57 @@ def action_muldelete(self, items):
self.update_redirect()
return redirect(self.get_redirect())
+ @action(
+ 'mulduplicate',
+ 'Duplicate',
+ 'Are you sure you want to duplicate the selected connections?',
+ single=False,
+ )
+ @provide_session
+ @auth.has_access(
+ [
+ (permissions.ACTION_CAN_CREATE, permissions.RESOURCE_CONNECTION),
+ ]
+ )
+ def action_mulduplicate(self, connections, session=None):
+ """Duplicate Multiple connections"""
+ for selected_conn in connections:
+ new_conn_id = selected_conn.conn_id
+ matches = re.findall(r"_copy(\d+)$", selected_conn.conn_id)
Review comment:
yeah, agree
**re.search()** will also match the last occurence.
using that.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] xinbinhuang edited a comment on pull request #15574: #12401 - Duplicating connections from UI
Posted by GitBox <gi...@apache.org>.
xinbinhuang edited a comment on pull request #15574:
URL: https://github.com/apache/airflow/pull/15574#issuecomment-831716841
I have the same feeling as @jhtimmins : should it be snakecase `_copy` instead?
Another thing is: can you also a test case to cover the duplicate logic?
Otherwise, the change looks neat 👍
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] pateash edited a comment on pull request #15574: Add means to Duplicate connections from UI
Posted by GitBox <gi...@apache.org>.
pateash edited a comment on pull request #15574:
URL: https://github.com/apache/airflow/pull/15574#issuecomment-912976507
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] pateash commented on pull request #15574: #12401 - Duplicating connections from UI
Posted by GitBox <gi...@apache.org>.
pateash commented on pull request #15574:
URL: https://github.com/apache/airflow/pull/15574#issuecomment-852977360
> Can you please rebase and solve conflicts @pateash ?
Thanks @potiuk , done.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] pateash commented on a change in pull request #15574: #12401 - Duplicating connections from UI
Posted by GitBox <gi...@apache.org>.
pateash commented on a change in pull request #15574:
URL: https://github.com/apache/airflow/pull/15574#discussion_r627176017
##########
File path: airflow/www/views.py
##########
@@ -3012,6 +3015,55 @@ def action_muldelete(self, items):
self.update_redirect()
return redirect(self.get_redirect())
+ @action(
+ 'mulduplicate',
+ 'Duplicate',
+ 'Are you sure you want to duplicate the selected connections?',
+ single=False,
+ )
+ @provide_session
+ @auth.has_access(
+ [
+ (permissions.ACTION_CAN_CREATE, permissions.RESOURCE_CONNECTION),
+ ]
+ )
+ def action_mulduplicate(self, connections, session=None):
+ """Duplicate Multiple connections"""
+ for selected_conn in connections:
+ new_conn_id = selected_conn.conn_id
+ match = re.search(r"_copy(\d+)$", selected_conn.conn_id)
+ if match:
+ conn_id_prefix = selected_conn.conn_id[: match.start()]
+ new_conn_id = f"{conn_id_prefix}_copy{int(match.group(1)) + 1}"
+ else:
+ new_conn_id += '_copy1'
+
+ dup_conn = Connection(
+ new_conn_id,
+ selected_conn.conn_type,
+ selected_conn.description,
+ selected_conn.host,
+ selected_conn.login,
+ selected_conn.password,
+ selected_conn.schema,
+ selected_conn.port,
+ selected_conn.extra,
+ )
+
+ try:
+ session.add(dup_conn)
+ session.commit()
+ flash(f"Connection {new_conn_id} added successfully.", "success")
+ except IntegrityError:
+ flash(
+ f"Connection {new_conn_id} can't be added. Integrity error, probably unique constraint.",
+ "warning",
+ )
+ session.rollback()
Review comment:
@uranusjr,
I think the current approach is better, as all the connections are independent to each other so they will get independent status.
Also, from an error message point of view, We are only catching integrity error, so we are providing the same error.
A similar approach is already in place in add() for the connection.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #15574: #12401 - Duplicating connections from UI
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #15574:
URL: https://github.com/apache/airflow/pull/15574#issuecomment-855732289
@ashb -> any more comments or can we merge it ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on a change in pull request #15574: #12401 - Duplicating connections from UI
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #15574:
URL: https://github.com/apache/airflow/pull/15574#discussion_r629904162
##########
File path: tests/www/views/test_views_connection.py
##########
@@ -54,3 +54,46 @@ def test_prefill_form_null_extra():
cmv = ConnectionModelView()
cmv.prefill_form(form=mock_form, pk=1)
+
+
+def test_duplicate_connection(admin_client):
+ """Test Duplicate multiple connection with suffix"""
+ conn1 = Connection(
+ conn_id='test_duplicate_gcp_connection',
+ conn_type='Google Cloud',
+ description='Google Cloud Connection',
+ )
+ conn2 = Connection(
+ conn_id='test_duplicate_mysql_connection',
+ conn_type='FTP',
+ description='MongoDB2',
+ host='localhost',
+ schema='airflow',
+ port=3306,
+ )
+ conn3 = Connection(
+ conn_id='test_duplicate_postgres_connection_copy1',
+ conn_type='FTP',
+ description='Postgres',
+ host='localhost',
+ schema='airflow',
+ port=3306,
+ )
+ with create_session() as session:
+ session.query(Connection).delete()
+ session.add_all([conn1, conn2, conn3])
+ session.commit()
Review comment:
I don’t agree with the distinction. IMO test setup should be test setup, and the test function should only contain the test itself. This makes tests easier to debug against the test framework; a test setup error shows up as `E` instead of `F` in pytet, for example. It also categories run durations differently (time spent on test setup is marked with “setup” in the summary) and makes it easier to detect slow tests.
But I guess you can argue that’s subjective.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on pull request #15574: Add means to Duplicate connections from UI
Posted by GitBox <gi...@apache.org>.
uranusjr commented on pull request #15574:
URL: https://github.com/apache/airflow/pull/15574#issuecomment-912999865
We should detect this and show a better error message. @jyotsa09 Could you file an issue for this?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] pateash commented on pull request #15574: Add means to Duplicate connections from UI
Posted by GitBox <gi...@apache.org>.
pateash commented on pull request #15574:
URL: https://github.com/apache/airflow/pull/15574#issuecomment-912976507
> There is a scenario where I feel this is not working as expected.
this is expected behaviour, you should not have any connection with _copy<X> in the end,
it's just a way to create duplicate, after duplication you are supposed to rename the connection.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] github-actions[bot] commented on pull request #15574: #12401 - Duplicating connections from UI
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #15574:
URL: https://github.com/apache/airflow/pull/15574#issuecomment-832590487
[The Workflow run](https://github.com/apache/airflow/actions/runs/813117646) is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jhtimmins commented on pull request #15574: #12401 - Duplicating connections from UI
Posted by GitBox <gi...@apache.org>.
jhtimmins commented on pull request #15574:
URL: https://github.com/apache/airflow/pull/15574#issuecomment-858117108
@ashb can you check whether your requested changes have been addressed?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] pateash commented on pull request #15574: #12401 - Duplicating connections from UI
Posted by GitBox <gi...@apache.org>.
pateash commented on pull request #15574:
URL: https://github.com/apache/airflow/pull/15574#issuecomment-828508657
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jyotsa09 edited a comment on pull request #15574: Add means to Duplicate connections from UI
Posted by GitBox <gi...@apache.org>.
jyotsa09 edited a comment on pull request #15574:
URL: https://github.com/apache/airflow/pull/15574#issuecomment-912438705
There is a scenario where I feel this is not working as expected.
- When I have created a connection with name **conn_test_copy1** and then I have tried to duplicate this connection. It actually created a connection with name as **conn_test_copy2** instead of **conn_test_copy1_copy1**.
Also in the above scenario if I am trying to duplicate the **conn_test_copy1** it's giving me an error of **Connection conn_test_copy2 can't be added. Integrity error, probably unique constraint**. It's because connection with name **conn_test_copy2** is already there but it's looking like we can't duplicate same connection twice.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] pateash commented on a change in pull request #15574: #12401 - Duplicating connections from UI
Posted by GitBox <gi...@apache.org>.
pateash commented on a change in pull request #15574:
URL: https://github.com/apache/airflow/pull/15574#discussion_r625874413
##########
File path: airflow/www/views.py
##########
@@ -3012,6 +3015,57 @@ def action_muldelete(self, items):
self.update_redirect()
return redirect(self.get_redirect())
+ @action(
+ 'mulduplicate',
+ 'Duplicate',
+ 'Are you sure you want to duplicate the selected connections?',
+ single=False,
+ )
+ @provide_session
+ @auth.has_access(
+ [
+ (permissions.ACTION_CAN_CREATE, permissions.RESOURCE_CONNECTION),
+ ]
+ )
+ def action_mulduplicate(self, connections, session=None):
+ """Duplicate Multiple connections"""
+ for selected_conn in connections:
+ new_conn_id = selected_conn.conn_id
+ matches = re.findall(r"_Copy\((\d+)\)", selected_conn.conn_id)
Review comment:
This is a good point,
In case of something like **abc_copy1_xyz**, we will currently end up doing **abc_copy2_xyz**, but **abc_copy1_xyz_copy1** should be the accepted behaviour.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] pateash commented on pull request #15574: #12401 - Duplicating connections from UI
Posted by GitBox <gi...@apache.org>.
pateash commented on pull request #15574:
URL: https://github.com/apache/airflow/pull/15574#issuecomment-851846914
> As others have said, we should have it as `_copy` not `_Copy`.
>
> Additionally, it should be `copy1` not `copy(1)` so it can be set via env vars if _has_ to. (You can't put `()` in env var names, so `AIRFLOW_CONN_FOO_COPY(1)` would not be possible
done
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] pateash commented on a change in pull request #15574: #12401 - Duplicating connections from UI
Posted by GitBox <gi...@apache.org>.
pateash commented on a change in pull request #15574:
URL: https://github.com/apache/airflow/pull/15574#discussion_r626794382
##########
File path: airflow/www/views.py
##########
@@ -3012,6 +3015,56 @@ def action_muldelete(self, items):
self.update_redirect()
return redirect(self.get_redirect())
+ @action(
+ 'mulduplicate',
+ 'Duplicate',
+ 'Are you sure you want to duplicate the selected connections?',
+ single=False,
+ )
+ @provide_session
+ @auth.has_access(
+ [
+ (permissions.ACTION_CAN_CREATE, permissions.RESOURCE_CONNECTION),
+ ]
+ )
+ def action_mulduplicate(self, connections, session=None):
+ """Duplicate Multiple connections"""
+ for selected_conn in connections:
+ new_conn_id = selected_conn.conn_id
+ matches = re.search(r"_copy(\d+)$", selected_conn.conn_id)
+ if matches:
+ new_conn_id = selected_conn.conn_id.replace(
+ f'_copy{matches.group(1)}', f'_copy{int(matches.group(1)) + 1}'
+ )
Review comment:
I agree,
that the name of variable should be singular.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on a change in pull request #15574: #12401 - Duplicating connections from UI
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #15574:
URL: https://github.com/apache/airflow/pull/15574#discussion_r625902690
##########
File path: airflow/www/views.py
##########
@@ -3012,6 +3015,57 @@ def action_muldelete(self, items):
self.update_redirect()
return redirect(self.get_redirect())
+ @action(
+ 'mulduplicate',
+ 'Duplicate',
+ 'Are you sure you want to duplicate the selected connections?',
+ single=False,
+ )
+ @provide_session
+ @auth.has_access(
+ [
+ (permissions.ACTION_CAN_CREATE, permissions.RESOURCE_CONNECTION),
+ ]
+ )
+ def action_mulduplicate(self, connections, session=None):
+ """Duplicate Multiple connections"""
+ for selected_conn in connections:
+ new_conn_id = selected_conn.conn_id
+ matches = re.findall(r"_copy(\d+)$", selected_conn.conn_id)
Review comment:
Since there can only be one match here, it’s probably better to do
```python
match = re.search(...)
if match is not None:
...
```
instead.
##########
File path: airflow/www/views.py
##########
@@ -3012,6 +3015,57 @@ def action_muldelete(self, items):
self.update_redirect()
return redirect(self.get_redirect())
+ @action(
+ 'mulduplicate',
+ 'Duplicate',
+ 'Are you sure you want to duplicate the selected connections?',
+ single=False,
+ )
+ @provide_session
+ @auth.has_access(
+ [
+ (permissions.ACTION_CAN_CREATE, permissions.RESOURCE_CONNECTION),
+ ]
+ )
+ def action_mulduplicate(self, connections, session=None):
+ """Duplicate Multiple connections"""
+ for selected_conn in connections:
+ new_conn_id = selected_conn.conn_id
+ matches = re.findall(r"_copy(\d+)$", selected_conn.conn_id)
+ if matches:
+ # replacing the last _Copy(Num) with _Copy(Num+1)
Review comment:
This comment is out of date.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on a change in pull request #15574: #12401 - Duplicating connections from UI
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #15574:
URL: https://github.com/apache/airflow/pull/15574#discussion_r627117778
##########
File path: airflow/www/views.py
##########
@@ -3012,6 +3015,55 @@ def action_muldelete(self, items):
self.update_redirect()
return redirect(self.get_redirect())
+ @action(
+ 'mulduplicate',
+ 'Duplicate',
+ 'Are you sure you want to duplicate the selected connections?',
+ single=False,
+ )
+ @provide_session
+ @auth.has_access(
+ [
+ (permissions.ACTION_CAN_CREATE, permissions.RESOURCE_CONNECTION),
+ ]
+ )
+ def action_mulduplicate(self, connections, session=None):
+ """Duplicate Multiple connections"""
+ for selected_conn in connections:
+ new_conn_id = selected_conn.conn_id
+ match = re.search(r"_copy(\d+)$", selected_conn.conn_id)
+ if match:
+ conn_id_prefix = selected_conn.conn_id[: match.start()]
+ new_conn_id = f"{conn_id_prefix}_copy{int(match.group(1)) + 1}"
+ else:
+ new_conn_id += '_copy1'
+
+ dup_conn = Connection(
+ new_conn_id,
+ selected_conn.conn_type,
+ selected_conn.description,
+ selected_conn.host,
+ selected_conn.login,
+ selected_conn.password,
+ selected_conn.schema,
+ selected_conn.port,
+ selected_conn.extra,
+ )
+
+ try:
+ session.add(dup_conn)
+ session.commit()
+ flash(f"Connection {new_conn_id} added successfully.", "success")
+ except IntegrityError:
+ flash(
+ f"Connection {new_conn_id} can't be added. Integrity error, probably unique constraint.",
+ "warning",
+ )
+ session.rollback()
Review comment:
Some thoughts on the current approach inserting the duplicated connections:
* This commits each duplicated connection separately, so any failures would not affect other duplications. I’m not sure I’d want this as a user (probably subjective).
* The error message is not very helpful either. We could make a bit more effort by querying whether there’s an existing connection to the ID, but that could be subject to race conditions as well? (I think some database-level configuration can avoid this?)
* Combining the previous two points, it may be possible to first connect all the new IDs, query whether any of them exist, and fail all of them cause conflicts or fail to insert.
* If there’s more than one failures, the user may get a bunch of flashes which may be difficult to read? But maybe that’s not really an issue in practice.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #15574: #12401 - Duplicating connections from UI
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #15574:
URL: https://github.com/apache/airflow/pull/15574#discussion_r649088108
##########
File path: airflow/www/views.py
##########
@@ -3113,6 +3116,55 @@ def action_muldelete(self, items):
self.update_redirect()
return redirect(self.get_redirect())
+ @action(
+ 'mulduplicate',
+ 'Duplicate',
+ 'Are you sure you want to duplicate the selected connections?',
+ single=False,
+ )
+ @provide_session
+ @auth.has_access(
+ [
+ (permissions.ACTION_CAN_CREATE, permissions.RESOURCE_CONNECTION),
Review comment:
```suggestion
(permissions.ACTION_CAN_CREATE, permissions.RESOURCE_CONNECTION),
(permissions.ACTION_CAN_READ, permissions.RESOURCE_CONNECTION),
```
Although it's unlikely you'd have create and not read, this should also have read permissions.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] pateash commented on a change in pull request #15574: #12401 - Duplicating connections from UI
Posted by GitBox <gi...@apache.org>.
pateash commented on a change in pull request #15574:
URL: https://github.com/apache/airflow/pull/15574#discussion_r627176017
##########
File path: airflow/www/views.py
##########
@@ -3012,6 +3015,55 @@ def action_muldelete(self, items):
self.update_redirect()
return redirect(self.get_redirect())
+ @action(
+ 'mulduplicate',
+ 'Duplicate',
+ 'Are you sure you want to duplicate the selected connections?',
+ single=False,
+ )
+ @provide_session
+ @auth.has_access(
+ [
+ (permissions.ACTION_CAN_CREATE, permissions.RESOURCE_CONNECTION),
+ ]
+ )
+ def action_mulduplicate(self, connections, session=None):
+ """Duplicate Multiple connections"""
+ for selected_conn in connections:
+ new_conn_id = selected_conn.conn_id
+ match = re.search(r"_copy(\d+)$", selected_conn.conn_id)
+ if match:
+ conn_id_prefix = selected_conn.conn_id[: match.start()]
+ new_conn_id = f"{conn_id_prefix}_copy{int(match.group(1)) + 1}"
+ else:
+ new_conn_id += '_copy1'
+
+ dup_conn = Connection(
+ new_conn_id,
+ selected_conn.conn_type,
+ selected_conn.description,
+ selected_conn.host,
+ selected_conn.login,
+ selected_conn.password,
+ selected_conn.schema,
+ selected_conn.port,
+ selected_conn.extra,
+ )
+
+ try:
+ session.add(dup_conn)
+ session.commit()
+ flash(f"Connection {new_conn_id} added successfully.", "success")
+ except IntegrityError:
+ flash(
+ f"Connection {new_conn_id} can't be added. Integrity error, probably unique constraint.",
+ "warning",
+ )
+ session.rollback()
Review comment:
@uranusjr,
I think the current approach is better, as all the connections are independent to each other so they will get independent status.
Also, from an error message point of view, We are only catching integrity error, so we are providing the same error.
A similar approach is already in place in add() and edit() functions for the Connection Model.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on a change in pull request #15574: #12401 - Duplicating connections from UI
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #15574:
URL: https://github.com/apache/airflow/pull/15574#discussion_r629904162
##########
File path: tests/www/views/test_views_connection.py
##########
@@ -54,3 +54,46 @@ def test_prefill_form_null_extra():
cmv = ConnectionModelView()
cmv.prefill_form(form=mock_form, pk=1)
+
+
+def test_duplicate_connection(admin_client):
+ """Test Duplicate multiple connection with suffix"""
+ conn1 = Connection(
+ conn_id='test_duplicate_gcp_connection',
+ conn_type='Google Cloud',
+ description='Google Cloud Connection',
+ )
+ conn2 = Connection(
+ conn_id='test_duplicate_mysql_connection',
+ conn_type='FTP',
+ description='MongoDB2',
+ host='localhost',
+ schema='airflow',
+ port=3306,
+ )
+ conn3 = Connection(
+ conn_id='test_duplicate_postgres_connection_copy1',
+ conn_type='FTP',
+ description='Postgres',
+ host='localhost',
+ schema='airflow',
+ port=3306,
+ )
+ with create_session() as session:
+ session.query(Connection).delete()
+ session.add_all([conn1, conn2, conn3])
+ session.commit()
Review comment:
I don’t agree with the distinction. IMO test setup should be test setup, and the test function should only contain the test itself. This makes tests easier to debug against the test framework; a test setup error shows up as `E` instead of `F` in pytet, for example. It also categories run durations differently (time spent on test setup is marked with “setup” in the summary) and makes it easier to detect slow tests.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] pateash commented on pull request #15574: #12401 - Duplicating connections from UI
Posted by GitBox <gi...@apache.org>.
pateash commented on pull request #15574:
URL: https://github.com/apache/airflow/pull/15574#issuecomment-828510941
It will show a user-friendly error in case of name conflict ( unique integrity)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] pateash commented on pull request #15574: #12401 - Duplicating connections from UI
Posted by GitBox <gi...@apache.org>.
pateash commented on pull request #15574:
URL: https://github.com/apache/airflow/pull/15574#issuecomment-837897954
@ryanahamilton @ashb
Could you please review and let me know if any more changes required.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] pateash commented on a change in pull request #15574: #12401 - Duplicating connections from UI
Posted by GitBox <gi...@apache.org>.
pateash commented on a change in pull request #15574:
URL: https://github.com/apache/airflow/pull/15574#discussion_r629893010
##########
File path: tests/www/views/test_views_connection.py
##########
@@ -54,3 +54,46 @@ def test_prefill_form_null_extra():
cmv = ConnectionModelView()
cmv.prefill_form(form=mock_form, pk=1)
+
+
+def test_duplicate_connection(admin_client):
+ """Test Duplicate multiple connection with suffix"""
+ conn1 = Connection(
+ conn_id='test_duplicate_gcp_connection',
+ conn_type='Google Cloud',
+ description='Google Cloud Connection',
+ )
+ conn2 = Connection(
+ conn_id='test_duplicate_mysql_connection',
+ conn_type='FTP',
+ description='MongoDB2',
+ host='localhost',
+ schema='airflow',
+ port=3306,
+ )
+ conn3 = Connection(
+ conn_id='test_duplicate_postgres_connection_copy1',
+ conn_type='FTP',
+ description='Postgres',
+ host='localhost',
+ schema='airflow',
+ port=3306,
+ )
+ with create_session() as session:
+ session.query(Connection).delete()
+ session.add_all([conn1, conn2, conn3])
+ session.commit()
+
+ mock_form = mock.Mock()
+ mock_form.data = {"action": "mulduplicate", "rowid": [conn1.id, conn3.id]}
+ resp = admin_client.post('/connection/action_post', data=mock_form.data, follow_redirects=True)
Review comment:
agree
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] github-actions[bot] commented on pull request #15574: Add means to Duplicate connections from UI
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #15574:
URL: https://github.com/apache/airflow/pull/15574#issuecomment-860498094
The PR is likely OK to be merged with just subset of tests for default Python and Database versions without running the full matrix of tests, because it does not modify the core of Airflow. If the committers decide that the full tests matrix is needed, they will add the label 'full tests needed'. Then you should rebase to the latest main or amend the last commit of the PR, and push it with --force-with-lease.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] pateash edited a comment on pull request #15574: #12401 - Duplicating connections from UI
Posted by GitBox <gi...@apache.org>.
pateash edited a comment on pull request #15574:
URL: https://github.com/apache/airflow/pull/15574#issuecomment-828508657
* all the connections will be suffixed with **_Copy(NUM)** similar to Windows file explorer
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] pateash commented on a change in pull request #15574: #12401 - Duplicating connections from UI
Posted by GitBox <gi...@apache.org>.
pateash commented on a change in pull request #15574:
URL: https://github.com/apache/airflow/pull/15574#discussion_r626438955
##########
File path: airflow/www/views.py
##########
@@ -3012,6 +3015,57 @@ def action_muldelete(self, items):
self.update_redirect()
return redirect(self.get_redirect())
+ @action(
+ 'mulduplicate',
+ 'Duplicate',
+ 'Are you sure you want to duplicate the selected connections?',
+ single=False,
+ )
+ @provide_session
+ @auth.has_access(
+ [
+ (permissions.ACTION_CAN_CREATE, permissions.RESOURCE_CONNECTION),
+ ]
+ )
+ def action_mulduplicate(self, connections, session=None):
+ """Duplicate Multiple connections"""
+ for selected_conn in connections:
+ new_conn_id = selected_conn.conn_id
+ matches = re.findall(r"_copy(\d+)$", selected_conn.conn_id)
Review comment:
yeah, agree as we are only matching the last occurence.
**re.search()** will be better.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] xinbinhuang commented on pull request #15574: #12401 - Duplicating connections from UI
Posted by GitBox <gi...@apache.org>.
xinbinhuang commented on pull request #15574:
URL: https://github.com/apache/airflow/pull/15574#issuecomment-831716841
I have the same feeling as @jhtimmins : should it be snakecase `_copy` instead?
Another thing is: can you also a test case to cover the duplicate logic?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #15574: #12401 - Duplicating connections from UI
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #15574:
URL: https://github.com/apache/airflow/pull/15574#discussion_r625731037
##########
File path: tests/www/test_views.py
##########
@@ -270,6 +270,73 @@ def test_prefill_form_null_extra(self):
cmv.prefill_form(form=mock_form, pk=1)
+class TestMultiDuplicateConnectionModelView(TestBase):
+ def setUp(self):
+ super().setUp()
+ conn1 = Connection(
+ conn_id='google_cloud_default',
+ conn_type='Google Cloud',
+ description='Default Google Cloud Connection',
+ )
+
+ # Already copied and modified
+ conn2 = Connection(
+ conn_id='mongodb',
+ conn_type='FTP',
+ description='MongoDB2',
+ host='localhost',
+ schema='airflow',
+ port='5567',
+ )
+
+ conn3 = Connection(
+ conn_id='mysql_connection',
+ conn_type='FTP',
+ description='MongoDB2',
+ host='localhost',
+ schema='airflow',
+ port='3306',
+ )
+
+ conn4 = Connection(
+ conn_id='postgres_connection_Copy(1)',
+ conn_type='FTP',
+ description='Postgres',
+ host='localhost',
+ schema='airflow',
+ port='3306',
+ )
+
+ self.clear_table(Connection)
+ self.session.add_all([conn1, conn2, conn3, conn4])
+ self.session.commit()
+
+ def tearDown(self):
+ self.clear_table(Connection)
+ super().tearDown()
+
+ def test_duplicate_connection(self):
+ """ Test Duplicate multiple connection with suffix"""
+
+ mock_form = mock.Mock()
+ mock_form.data = {"action": "mulduplicate", "rowid": [1, 2, 4]}
+ resp = self.client.post('/connection/action_post', data=mock_form.data, follow_redirects=True)
+
+ expected_result = {
+ 'google_cloud_default',
+ 'google_cloud_default_Copy(1)',
+ 'mongodb',
+ 'mongodb_Copy(1)',
+ 'mysql_connection',
+ 'postgres_connection_Copy(1)',
+ 'postgres_connection_Copy(2)',
+ }
+ response = {conn[0].strip() for conn in self.session.query(Connection.conn_id).all()}
Review comment:
```suggestion
response = {conn[0] for conn in self.session.query(Connection.conn_id).all()}
```
strip shouldn't be needed.
##########
File path: tests/www/test_views.py
##########
@@ -270,6 +270,73 @@ def test_prefill_form_null_extra(self):
cmv.prefill_form(form=mock_form, pk=1)
+class TestMultiDuplicateConnectionModelView(TestBase):
Review comment:
Please don't create a new test class -- instead add methods to the existing TestConnectionModelView
##########
File path: tests/www/test_views.py
##########
@@ -561,15 +626,15 @@ def test_health(self):
BaseJob.job_type == 'SchedulerJob',
BaseJob.state == 'running',
BaseJob.latest_heartbeat == last_scheduler_heartbeat_for_testing_1,
- ).delete()
+ ).delete()
Review comment:
Please avoid making formatting only changes to lines you don't otherwise touch, as it makes the git history messier.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] pateash commented on a change in pull request #15574: #12401 - Duplicating connections from UI
Posted by GitBox <gi...@apache.org>.
pateash commented on a change in pull request #15574:
URL: https://github.com/apache/airflow/pull/15574#discussion_r626439776
##########
File path: airflow/www/views.py
##########
@@ -3012,6 +3015,57 @@ def action_muldelete(self, items):
self.update_redirect()
return redirect(self.get_redirect())
+ @action(
+ 'mulduplicate',
+ 'Duplicate',
+ 'Are you sure you want to duplicate the selected connections?',
+ single=False,
+ )
+ @provide_session
+ @auth.has_access(
+ [
+ (permissions.ACTION_CAN_CREATE, permissions.RESOURCE_CONNECTION),
+ ]
+ )
+ def action_mulduplicate(self, connections, session=None):
+ """Duplicate Multiple connections"""
+ for selected_conn in connections:
+ new_conn_id = selected_conn.conn_id
+ matches = re.findall(r"_copy(\d+)$", selected_conn.conn_id)
+ if matches:
+ # replacing the last _Copy(Num) with _Copy(Num+1)
Review comment:
removed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jyotsa09 commented on pull request #15574: Add means to Duplicate connections from UI
Posted by GitBox <gi...@apache.org>.
jyotsa09 commented on pull request #15574:
URL: https://github.com/apache/airflow/pull/15574#issuecomment-912438705
There is a scenario where I feel this is not working as expected.
- When I have created a connection with name **conn_test_copy1** and then I have tried to duplicate this connection. It actually created a connection with name as **conn_test_copy2** instead of **conn_test_copy1_copy1**.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #15574: #12401 - Duplicating connections from UI
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #15574:
URL: https://github.com/apache/airflow/pull/15574#discussion_r625870078
##########
File path: tests/www/test_views.py
##########
@@ -561,15 +626,15 @@ def test_health(self):
BaseJob.job_type == 'SchedulerJob',
BaseJob.state == 'running',
BaseJob.latest_heartbeat == last_scheduler_heartbeat_for_testing_1,
- ).delete()
+ ).delete()
Review comment:
@pateash No problem, I know how it happens, it just makes my life as a release manager much harder :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jyotsa09 edited a comment on pull request #15574: Add means to Duplicate connections from UI
Posted by GitBox <gi...@apache.org>.
jyotsa09 edited a comment on pull request #15574:
URL: https://github.com/apache/airflow/pull/15574#issuecomment-912438705
There is a scenario where I feel this is not working as expected.
- When I have created a connection with name **conn_test_copy1** and then I have tried to duplicate this connection. It actually created a connection with name as **conn_test_copy2** instead of **conn_test_copy1_copy1**.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jhtimmins commented on pull request #15574: #12401 - Duplicating connections from UI
Posted by GitBox <gi...@apache.org>.
jhtimmins commented on pull request #15574:
URL: https://github.com/apache/airflow/pull/15574#issuecomment-831661455
@ryanahamilton @ashb Bumping y'all on this since you're code owners here.
I think the code looks solid (nice job on the string update logic @pateash). My only nitpick is if `_Copy` should be uppercased. If connections typically have snakecase titles, it'd be cleaner to go lowercase (`_copy`). But if no one else minds then I'm fine with it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] pateash commented on a change in pull request #15574: #12401 - Duplicating connections from UI
Posted by GitBox <gi...@apache.org>.
pateash commented on a change in pull request #15574:
URL: https://github.com/apache/airflow/pull/15574#discussion_r629898446
##########
File path: tests/www/views/test_views_connection.py
##########
@@ -54,3 +54,46 @@ def test_prefill_form_null_extra():
cmv = ConnectionModelView()
cmv.prefill_form(form=mock_form, pk=1)
+
+
+def test_duplicate_connection(admin_client):
+ """Test Duplicate multiple connection with suffix"""
+ conn1 = Connection(
+ conn_id='test_duplicate_gcp_connection',
+ conn_type='Google Cloud',
+ description='Google Cloud Connection',
+ )
+ conn2 = Connection(
+ conn_id='test_duplicate_mysql_connection',
+ conn_type='FTP',
+ description='MongoDB2',
+ host='localhost',
+ schema='airflow',
+ port=3306,
+ )
+ conn3 = Connection(
+ conn_id='test_duplicate_postgres_connection_copy1',
+ conn_type='FTP',
+ description='Postgres',
+ host='localhost',
+ schema='airflow',
+ port=3306,
+ )
+ with create_session() as session:
+ session.query(Connection).delete()
+ session.add_all([conn1, conn2, conn3])
+ session.commit()
Review comment:
Yeah, But this is very specific to this individual test case.
if we add a fixture, we should use this in at least 2 places.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on a change in pull request #15574: #12401 - Duplicating connections from UI
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #15574:
URL: https://github.com/apache/airflow/pull/15574#discussion_r629886591
##########
File path: tests/www/views/test_views_connection.py
##########
@@ -54,3 +54,46 @@ def test_prefill_form_null_extra():
cmv = ConnectionModelView()
cmv.prefill_form(form=mock_form, pk=1)
+
+
+def test_duplicate_connection(admin_client):
+ """Test Duplicate multiple connection with suffix"""
+ conn1 = Connection(
+ conn_id='test_duplicate_gcp_connection',
+ conn_type='Google Cloud',
+ description='Google Cloud Connection',
+ )
+ conn2 = Connection(
+ conn_id='test_duplicate_mysql_connection',
+ conn_type='FTP',
+ description='MongoDB2',
+ host='localhost',
+ schema='airflow',
+ port=3306,
+ )
+ conn3 = Connection(
+ conn_id='test_duplicate_postgres_connection_copy1',
+ conn_type='FTP',
+ description='Postgres',
+ host='localhost',
+ schema='airflow',
+ port=3306,
+ )
+ with create_session() as session:
+ session.query(Connection).delete()
+ session.add_all([conn1, conn2, conn3])
+ session.commit()
+
+ mock_form = mock.Mock()
+ mock_form.data = {"action": "mulduplicate", "rowid": [conn1.id, conn3.id]}
+ resp = admin_client.post('/connection/action_post', data=mock_form.data, follow_redirects=True)
Review comment:
Is `mock_form` needed? It doesn’t seem to be used anywhere.
It seems only this is needed:
```python
data = {"action": "mulduplicate", "rowid": [conn1.id, conn3.id]}
resp = admin_client.post('/connection/action_post', data=data, follow_redirects=True)
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org