Skip to content

feat: add support for SQS FIFO Queue#4014

Merged
mergify[bot] merged 22 commits intoaws:mainlinefrom
paragbhingre:sssFIFOQueue
Sep 23, 2022
Merged

feat: add support for SQS FIFO Queue#4014
mergify[bot] merged 22 commits intoaws:mainlinefrom
paragbhingre:sssFIFOQueue

Conversation

@paragbhingre
Copy link
Copy Markdown
Contributor

@paragbhingre paragbhingre commented Sep 20, 2022

This PR resolves #3828 , resolves #2800

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the Apache 2.0 License.

@paragbhingre paragbhingre requested a review from a team as a code owner September 20, 2022 00:52
@paragbhingre paragbhingre requested review from KollaAdithya and removed request for a team September 20, 2022 00:52
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Sep 20, 2022

Codecov Report

Merging #4014 (52ef8f5) into mainline (66a7fc8) will increase coverage by 0.11%.
The diff coverage is 82.94%.

❗ Current head 52ef8f5 differs from pull request most recent head 9b811c6. Consider uploading reports for the commit 9b811c6 to get more accurate results

@@             Coverage Diff              @@
##           mainline    #4014      +/-   ##
============================================
+ Coverage     68.85%   68.96%   +0.11%     
============================================
  Files           248      248              
  Lines         35125    35248     +123     
  Branches        264      264              
============================================
+ Hits          24184    24310     +126     
+ Misses         9751     9748       -3     
  Partials       1190     1190              
Impacted Files Coverage Δ
internal/pkg/manifest/workload.go 82.81% <ø> (ø)
internal/pkg/template/template_functions.go 81.72% <0.00%> (-1.80%) ⬇️
internal/pkg/template/workload.go 38.73% <ø> (ø)
internal/pkg/cli/deploy/svc.go 48.17% <50.00%> (ø)
internal/pkg/manifest/worker_svc.go 75.86% <59.37%> (-0.57%) ⬇️
internal/pkg/manifest/validate.go 84.22% <98.03%> (+2.34%) ⬆️
...al/pkg/deploy/cloudformation/stack/transformers.go 76.67% <100.00%> (+1.37%) ⬆️
...rnal/pkg/deploy/cloudformation/stack/worker_svc.go 59.67% <100.00%> (ø)
internal/pkg/cli/app_init.go 58.54% <0.00%> (-0.77%) ⬇️
... and 4 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Comment thread internal/pkg/deploy/cloudformation/stack/transformers.go Outdated
Comment thread internal/pkg/deploy/cloudformation/stack/transformers.go Outdated
Comment thread internal/pkg/deploy/cloudformation/stack/transformers.go Outdated
Comment thread internal/pkg/deploy/cloudformation/stack/transformers.go
Comment thread internal/pkg/manifest/validate.go Outdated
Comment thread internal/pkg/manifest/worker_svc.go Outdated
Comment thread internal/pkg/manifest/workload.go Outdated
Comment thread internal/pkg/template/template_functions.go Outdated
Comment thread internal/pkg/template/templates/workloads/partials/cf/subscribe.yml
Comment thread internal/pkg/template/templates/workloads/partials/cf/subscribe.yml Outdated
Comment thread internal/pkg/template/workload.go Outdated
Comment thread internal/pkg/manifest/validate.go Outdated
Comment thread internal/pkg/manifest/validate.go Outdated
Comment thread internal/pkg/manifest/worker_svc.go Outdated
Comment thread internal/pkg/deploy/cloudformation/stack/transformers.go Outdated
Comment on lines +885 to +889
if aws.BoolValue(q.FIFO.Enable) {
queue.FIFOQueueConfig = &template.FIFOQueueConfig{
Enable: q.FIFO.Enable,
}
return queue
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can we do like SNS and not have Enable as field in the template type:

Suggested change
if aws.BoolValue(q.FIFO.Enable) {
queue.FIFOQueueConfig = &template.FIFOQueueConfig{
Enable: q.FIFO.Enable,
}
return queue
queue.FIFOQueueConfig = &template.FIFOQueueConfig{}
if q.FIFO.IsEnabled() && q.FIFO.Advanced.IsEmpty() {
return queue

Copy link
Copy Markdown
Contributor Author

@paragbhingre paragbhingre Sep 22, 2022

Choose a reason for hiding this comment

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

Yes I was working on the same thing here when I got that comment in SNS PR. The change is coming in the latest commit.

I already have this in place -

         if !q.FIFO.IsEnabled() {
		return queue
	}

	if aws.BoolValue(q.FIFO.Enable) {
		queue.FIFOQueueConfig = &template.FIFOQueueConfig{}
		return queue
	}
       if !q.FIFO.Advanced.IsEmpty() {
                // advanced config ...
       }

Let me know if that works

Comment thread internal/pkg/deploy/cloudformation/stack/transformers.go Outdated
Comment thread internal/pkg/deploy/cloudformation/stack/transformers.go Outdated
Comment thread internal/pkg/deploy/cloudformation/stack/transformers.go Outdated
Comment thread internal/pkg/manifest/validate.go Outdated
Comment thread internal/pkg/manifest/worker_svc.go Outdated
Comment thread internal/pkg/manifest/workload.go Outdated
Comment thread internal/pkg/template/templates/workloads/partials/cf/subscribe.yml
Comment thread internal/pkg/template/workload.go
Comment thread internal/pkg/template/workload.go Outdated
Comment thread internal/pkg/template/templates/workloads/partials/cf/subscribe.yml
Comment thread internal/pkg/manifest/worker_svc.go Outdated
Comment thread internal/pkg/manifest/worker_svc.go Outdated
Comment thread internal/pkg/manifest/validate.go Outdated
Comment thread internal/pkg/manifest/validate.go Outdated
Copy link
Copy Markdown
Contributor

@efekarakus efekarakus left a comment

Choose a reason for hiding this comment

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

A tiny change and then feel free to remove the label

Comment thread internal/pkg/cli/deploy/svc.go Outdated
@efekarakus efekarakus added the do-not-merge Pull requests that mergify shouldn't merge until the requester allows it. label Sep 23, 2022
Copy link
Copy Markdown
Contributor

@Lou1415926 Lou1415926 left a comment

Choose a reason for hiding this comment

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

LGTM just one final question!

Comment thread internal/pkg/manifest/validate.go Outdated
Comment thread internal/pkg/manifest/validate.go Outdated
Comment thread internal/pkg/manifest/worker_svc.go Outdated
Comment thread internal/pkg/manifest/worker_svc.go Outdated
Copy link
Copy Markdown
Contributor

@Lou1415926 Lou1415926 left a comment

Choose a reason for hiding this comment

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

:shipit:

@efekarakus efekarakus removed the do-not-merge Pull requests that mergify shouldn't merge until the requester allows it. label Sep 23, 2022
@mergify mergify Bot merged commit b3a78b9 into aws:mainline Sep 23, 2022
@paragbhingre paragbhingre deleted the sssFIFOQueue branch January 26, 2023 07:29
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.

Still no FIFO SNS/SQS? Allow worker pattern to create FIFO queue

5 participants