-
Notifications
You must be signed in to change notification settings - Fork 28
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
Task status, heavy write fix and aws ips #322
Conversation
I would put all that logic inside tpi stop. That would examine the logs and determine if we are OK, failed, etc... |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
* Reduces S3 writes
1ede17d
to
71b9f77
Compare
be39fea
to
71129b7
Compare
Teststerraform {
required_providers {
iterative = {
source = "github.com/iterative/iterative"
}
}
}
provider "iterative" {}
resource "iterative_task" "succeeded" {
name = "succeeded"
cloud = "aws"
script = "#!/bin/sh\nexit 0"
}
resource "iterative_task" "failed" {
name = "failed"
cloud = "aws"
script = "#!/bin/sh\nexit 1"
}
resource "iterative_task" "running" {
name = "running"
cloud = "aws"
script = "#!/bin/sh\nsleep infinity"
} Results$ terraform show
# iterative_task.succeeded:
resource "iterative_task" "succeeded" {
···
status = {
"running" = 0
"succeeded" = 1
}
}
# iterative_task.failed:
resource "iterative_task" "failed" {
···
status = {
"failed" = 1
"running" = 0
}
}
# iterative_task.running:
resource "iterative_task" "running" {
···
status = {
"running" = 1
}
} |
@0x2b3bfa0 |
Yes, |
Sounds good. A nitpick would be that I prefer one status vs a composed status... but both are equivalent to some extend |
😅 It happened because DescribeInstances with InstanceIds = [] was the same as describing every imaginable instance in the account.
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.
Fast nitpicks while finishing review
@@ -26,7 +26,9 @@ type Status map[StatusCode]int | |||
type StatusCode string | |||
|
|||
const ( | |||
StatusCodeRunning StatusCode = "running" | |||
StatusCodeActive StatusCode = "running" | |||
StatusCodeSucceeded StatusCode = "succeeded" |
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.
success
sounds better and a more widely used word in APIs
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.
Tell that to Kubernetes developers. 😈 I'm being a bit facetious, of course, and agree that succeeded sounds a bit strange, but it's probably the only grammatically consistent option for that term. 😅
🔔 @casperdcl and your dictionary of synonyms.
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.
but it's probably the only grammatically consistent option for that term.
Also success
How would that “one status” look like? Sounds interesting. |
We have already design this previously. As stated in #319 It could even be codes like HTTP status codes... so extensible, more at least than two booleans |
task
status field is not consistent enough #319; see #318 for an explanation of theExecStart=-
syntax.task
Write and Read to S3 is heavy #321 with (guess it) purebash
task
tfstate shows all the ips from other tasks #297 with b7520b0 unless proven otherwise.task
creation does not complete #330