-
Notifications
You must be signed in to change notification settings - Fork 398
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ecs_service_info lists all services in all clusters when no cluster attribute is given #2058
base: main
Are you sure you want to change the base?
Conversation
Build failed. ❌ ansible-galaxy-importer FAILURE in 5m 02s (non-voting) |
Docs Build 📝Thank you for contribution!✨ The docsite for this PR is available for download as an artifact from this run: You can compare to the docs for the File changes:
Click to see the diff comparison.NOTE: only file modifications are shown here. New and deleted files are excluded. diff --git a/home/runner/work/community.aws/community.aws/docsbuild/base/collections/community/aws/ecs_service_info_module.html b/home/runner/work/community.aws/community.aws/docsbuild/head/collections/community/aws/ecs_service_info_module.html
index c8095dd..be7018a 100644
--- a/home/runner/work/community.aws/community.aws/docsbuild/base/collections/community/aws/ecs_service_info_module.html
+++ b/home/runner/work/community.aws/community.aws/docsbuild/head/collections/community/aws/ecs_service_info_module.html
@@ -215,9 +215,9 @@ see <a class="reference internal" href="#ansible-collections-community-aws-ecs-s
</tr>
<tr class="row-odd"><td><div class="ansible-option-cell">
<div class="ansibleOptionAnchor" id="parameter-cluster"></div><p class="ansible-option-title" id="ansible-collections-community-aws-ecs-service-info-module-parameter-cluster"><strong>cluster</strong></p>
-<a class="ansibleOptionLink" href="#parameter-cluster" title="Permalink to this option"></a><p class="ansible-option-type-line"><span class="ansible-option-type">string</span></p>
+<a class="ansibleOptionLink" href="#parameter-cluster" title="Permalink to this option"></a><p class="ansible-option-type-line"><span class="ansible-option-type">list</span> / <span class="ansible-option-elements">elements=string</span></p>
</div></td>
-<td><div class="ansible-option-cell"><p>The cluster ARNS in which to list the services.</p>
+<td><div class="ansible-option-cell"><p>The cluster ARNS in which to list the services. If not provided, all clusters are listed.</p>
</div></td>
</tr>
<tr class="row-even"><td><div class="ansible-option-cell">
@@ -321,7 +321,7 @@ see <a class="reference internal" href="#ansible-collections-community-aws-ecs-s
<a class="ansibleOptionLink" href="#parameter-service" title="Permalink to this option"></a><p class="ansible-option-type-line"><span class="ansible-option-aliases">aliases: name</span></p>
<p class="ansible-option-type-line"><span class="ansible-option-type">list</span> / <span class="ansible-option-elements">elements=string</span></p>
</div></td>
-<td><div class="ansible-option-cell"><p>One or more services to get details for</p>
+<td><div class="ansible-option-cell"><p>One or more services to get details for. If not provided, all services are listed.</p>
</div></td>
</tr>
<tr class="row-even"><td><div class="ansible-option-cell">
@@ -379,9 +379,15 @@ see <a class="reference internal" href="#ansible-collections-community-aws-ecs-s
<span class="w"> </span><span class="nt">details</span><span class="p">:</span><span class="w"> </span><span class="l l-Scalar l-Scalar-Plain">true</span>
<span class="w"> </span><span class="nt">register</span><span class="p">:</span><span class="w"> </span><span class="l l-Scalar l-Scalar-Plain">output</span>
-<span class="c1"># Basic listing example</span>
+<span class="c1"># Basic listing example for all services in all clusters</span>
<span class="p p-Indicator">-</span><span class="w"> </span><span class="nt">community.aws.ecs_service_info</span><span class="p">:</span>
-<span class="w"> </span><span class="nt">cluster</span><span class="p">:</span><span class="w"> </span><span class="l l-Scalar l-Scalar-Plain">test-cluster</span>
+<span class="w"> </span><span class="nt">details</span><span class="p">:</span><span class="w"> </span><span class="l l-Scalar l-Scalar-Plain">true</span>
+<span class="w"> </span><span class="nt">register</span><span class="p">:</span><span class="w"> </span><span class="l l-Scalar l-Scalar-Plain">output</span>
+<span class="c1"># Basic listing example for the list of services in two specific clusters</span>
+<span class="p p-Indicator">-</span><span class="w"> </span><span class="nt">community.aws.ecs_service_info</span><span class="p">:</span>
+<span class="w"> </span><span class="nt">cluster</span><span class="p">:</span>
+<span class="w"> </span><span class="p p-Indicator">-</span><span class="w"> </span><span class="l l-Scalar l-Scalar-Plain">test-cluster</span>
+<span class="w"> </span><span class="p p-Indicator">-</span><span class="w"> </span><span class="l l-Scalar l-Scalar-Plain">prod-cluster</span>
<span class="w"> </span><span class="nt">register</span><span class="p">:</span><span class="w"> </span><span class="l l-Scalar l-Scalar-Plain">output</span>
</pre></div>
</div>
|
Build failed. ❌ ansible-galaxy-importer FAILURE in 5m 20s (non-voting) |
Build failed. ✔️ ansible-galaxy-importer SUCCESS in 5m 31s (non-voting) |
…rs, returning a non-existent service in a cluster result does not make sense.
Build failed. ❌ ansible-galaxy-importer FAILURE in 5m 09s (non-voting) |
Build succeeded. ✔️ ansible-galaxy-importer SUCCESS in 4m 54s (non-voting) |
Co-authored-by: Markus Bergholz <[email protected]>
Co-authored-by: Markus Bergholz <[email protected]>
Co-authored-by: Markus Bergholz <[email protected]>
Build succeeded. ❌ ansible-galaxy-importer FAILURE in 5m 27s (non-voting) |
recheck |
Build succeeded. ✔️ ansible-galaxy-importer SUCCESS in 5m 01s (non-voting) |
hm, the failing CI jobs are not related to this PR
|
else: | ||
clusters = self.list_clusters() | ||
|
||
cluster_services = {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cluster_services = {} | |
cluster_services = {} |
Service name per cluster are unique. What will happen when more than one cluster contains the same service name?
Will the last service name overwrite the first service name?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there is an issue when the details flag is not added or set to false. This would return just the list of services with no attributes. Considering the scenario of 2 clusters, with the same HelloWorld service.
- community.aws.ecs_service_info:
details: false
..would get you a list of 2 service ARNs
- community.aws.ecs_service_info:
service: arn:aws:ecs:eu-central-1:123456789012:service/my-cluster/HelloWorld
..would get you a single ARN
But giving service names (not service ARNs) with details: false in the current ecs_service_info returns you just the existing services. So in this scenario:
- community.aws.ecs_service_info:
details: false
service: HelloWorld
..would in the current module fail, or force you to add a cluster and return just [HelloWorld]. But what should be the response if you have the given HelloWorld scenario? [HelloWorld] is ambiguous right? But adding cluster info or returning a cluster:service dict would be a change that changes the module response compared to the current version.
I am not an experienced contributer. Do you have a suggestion how to proceed here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah maybe I also misinterpreted the return value
cluster_services[cluster] = ...
So it's already grouped by cluster.
returning a cluster:service dict would be a change that changes the module response compared to the current version.
I think this is the only solution. That would also mean that it is a breaking change and the changes will release with 8.0.0 and not in 7.2.0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay. I need some guidance in what would be good form for the output.
So (with details: false) the current output:
services:
- hello_world-1
- hello_world-2
If I were to slide in the clusters I could add them as an extra level below services:
services:
my-cluster:
- hello_world-1
- hello_world-2
my-other-cluster:
- hello_world-1
- hello_world-2
Or should this be more explicit (and renaming the root item) in the lines of:
result:
- cluster: my-cluster
services:
- hello_world-1
- hello_world-2
- cluster: my-other-cluster
services:
- hello_world-1
- hello_world-2
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
any opinions @tremble @alinabuzachis?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, current return value for details: false
is
services:
- hello_world-1
- hello_world-2
using details: true
results in
services:
- clusterArn: ...blue
serviceName: hello_world-1
....
- clusterArn: ...red
serviceName: hello_world-1
...
I guess we should keep this structure.
And once cluster
name is omit
, details
should be automatically set to true
- name: test
register: out
ecs_service_info:
cluster: "{{ omit }}"
details: true
This won't break any backwards compatiblity and won't return any ambiguous results in case a service name exist in more than one cluster.
What do you think @b0tting
Fixed by #2059 Short version, there was a bug in the sanity tests, fix was merged a while back but it too time before milestone was updated. |
Build succeeded. ❌ ansible-galaxy-importer FAILURE in 4m 47s (non-voting) |
SUMMARY
Added a function to internally query clusters to allow listing of all services without requiring a cluster name in the task.
The current version of ecs_service_info requires a cluster name. There is no ansible task option to list cluster names requiring us to use a command task. By internally querying the clusters we can list all services without needing to know a cluster name. When a cluster ARN (or a list of cluster ARNs) is given only the services for those clusters will be included in the response
Fixes #2056
ISSUE TYPE
COMPONENT NAME
ecs_service_info
ADDITIONAL INFORMATION
As suggested by @markuman in #2056 (suggestion 1)
Task response example without having to add a cluster:
..Ansible task:
..response: