-
Notifications
You must be signed in to change notification settings - Fork 618
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
Log driver secret support functional test #1952
Conversation
2bec351
to
905bf74
Compare
905bf74
to
8627a81
Compare
Here is the link to the approved PR for the code change: |
8627a81
to
35538f5
Compare
35538f5
to
efb8e11
Compare
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.
can you check why tests are all failing? Also pleas update your commit message to reflect your change for ecs model.
// create parameter in parameter store if it does not exist | ||
_, err = ssmClient.PutParameter(input) | ||
if err != nil { | ||
if aerr, ok := err.(awserr.Error); ok { |
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.
You can prob change to this:
if aerr, ok := err.(awserr.Error); ok && aerr.Code() == ssm.ErrCodeParameterAlreadyExists {
your logic goes 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.
nit: miss a newline
t.Logf("Parameter %v already exists in SSM Parameter Store", parameterName) | ||
break | ||
default: | ||
require.NoError(t, err, "SSM PutParameter call failed") |
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.
can you use t.Errorf instead to make the logic clear that it will error if execute to this line. since you already check that err != nil
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.
Since this block of code resides in several test cases being involved in creating parameter in parameter store if not exist, I will leave this for now and try to optimize it all together later on.
defer os.Unsetenv("ECS_FTEST_FORCE_NET_HOST") | ||
|
||
agent := RunAgent(t, &agentOptions) | ||
|
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.
nit: extra space
@@ -0,0 +1,26 @@ | |||
{ | |||
"family": "test-awslogs-secret-multiline", |
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.
curious, what does this multiline mean in this test exactly?
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.
To test if the secret is supported in a log driver generally, e.g. awslogs. In this case, secret will only be supported in optional awslogs log driver options, and only awslogs-multiline-pattern and awslogs-datetime-format options meet this requirement for both EC2 and Fargate launch type.
For awslogs-multiline-pattern:
"This option defines a multiline start pattern using a regular expression. A log message consists of a line that matches the pattern and any following lines that don’t match the pattern. Thus the matched line is the delimiter between log messages."
Check out this link to see more details:
https://docs.aws.amazon.com/AmazonECS/latest/developerguide/using_awslogs.html
Hope it helps :)
efb8e11
to
cf69d34
Compare
eacb766
to
5346e97
Compare
cf69d34
to
6bf925a
Compare
Can you take a look that why arm functional test failed? |
6bf925a
to
ba3b1e7
Compare
Summary
Log driver secret support functional test using awslogs' multiline-pattern option.
Implementation details
Testing
New tests cover the changes:
Description for the changelog
Licensing
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.