Skip to content

Conversation

@mattaltberg
Copy link
Contributor

With this, someone can use attach_scheduling_policy in their job_queue definition to skip over scheduling policy attachment.

Description

Right now, a scheduling policy is auto-attached to all job queues. Adding "attach_scheduling_policy" when defining people's job queues will override the default behaviour.

Motivation and Context

I don't want to always have a scheduling policy for my queues.

Breaking Changes

No, it uses lookup, and defaults to the original behaviour

How Has This Been Tested?

  • I have updated at least one of the examples/* to demonstrate and validate my change(s)
  • I have tested and validated these changes using one or more of the provided examples/* projects
  • I have executed pre-commit run -a on my pull request

@mattaltberg mattaltberg changed the title Add attach_scheduling_policy option to job_queue feat: Add attach_scheduling_policy option to job_queue Jul 4, 2022
@mattaltberg
Copy link
Contributor Author

Hey @bryantbiggs @antonbabenko can I get this added as well? I want to add job queues that don't have scheduling policies.

@mattaltberg
Copy link
Contributor Author

Hey @bryantbiggs, did you see my comment?

@bryantbiggs
Copy link
Member

yes, I started to take a look and haven't come up with a solution that seems reasonable yet

@mattaltberg
Copy link
Contributor Author

It's tricky, we need a way to say we don't want the scheduling policy, and it has to be at the queue level. Curious to see what you come up with

main.tf Outdated
state = each.value.state
priority = each.value.priority
scheduling_policy_arn = try(each.value.scheduling_policy_arn, aws_batch_scheduling_policy.this[each.key].arn)
scheduling_policy_arn = lookup(each.value, "attach_scheduling_policy", true) ? try(each.value.scheduling_policy_arn, aws_batch_scheduling_policy.this[each.key].arn) : null
Copy link
Member

Choose a reason for hiding this comment

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

lets change this to

Suggested change
scheduling_policy_arn = lookup(each.value, "attach_scheduling_policy", true) ? try(each.value.scheduling_policy_arn, aws_batch_scheduling_policy.this[each.key].arn) : null
scheduling_policy_arn = try(each.value.create_scheduling_policy, true) ? aws_batch_scheduling_policy.this[each.key].arn : try(each.value.scheduling_policy_arn, null)

then down on line 237 (can't comment on lines not in diff) - lets change line 237 to

  for_each = { for k, v in var.job_queues : k => v if var.create && var.create_job_queues && try(v.create_scheduling_policy, true) }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed, I didn't actually know you could use values in the for_each

@mattaltberg
Copy link
Contributor Author

@bryantbiggs I actually found a workaround, you can supply scheduling_policy_arn = null when you define your job queue, and it works. Not sure this change is needed anymore (although scheduling policies will still be created)

@mattaltberg mattaltberg changed the title feat: Add attach_scheduling_policy option to job_queue feat: Add create_scheduling_policy option to job_queue Jul 5, 2022
With this, someone can use attach_scheduling_policy in their job_queue definition to skip over scheduling policy attachment.
@mattaltberg
Copy link
Contributor Author

@bryantbiggs done and ready

Copy link
Member

@bryantbiggs bryantbiggs left a comment

Choose a reason for hiding this comment

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

thanks @mattaltberg !

@bryantbiggs bryantbiggs merged commit 86546af into terraform-aws-modules:master Jul 19, 2022
antonbabenko pushed a commit that referenced this pull request Jul 19, 2022
## [1.2.0](v1.1.2...v1.2.0) (2022-07-19)

### Features

* Add create_scheduling_policy option to job_queue ([#3](#3)) ([86546af](86546af))
@antonbabenko
Copy link
Member

This PR is included in version 1.2.0 🎉

@mattaltberg mattaltberg deleted the patch-2 branch July 19, 2022 13:41
@github-actions
Copy link

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 28, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants