Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Provide
gitlab_pages_domain
resource #1026Provide
gitlab_pages_domain
resource #1026Changes from all commits
52a315a
147af90
e6acf6b
0f48a69
0f2129e
e62cce3
fb5bebb
d78cc20
f16cf4c
179f735
9e2bc9e
1645b1f
578411f
acd210f
e3f79ff
4c63db5
5ff3b52
8dd4266
ad207eb
db670b7
1afd556
c8d2853
d7234c4
aa03e9e
6deef5a
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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: to indicate that we only use these attributes for the GitLab options I think it makes sense to not have variables for them ...
Please verify the correct behavior during creation if these optional attributes are not given and that the GitLab API doesn't receive them ....
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.
Do you mean separate testcases? The current test does not specify any of the optional attributes.
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.
True, noticed afterwards - let's make sure that both scenarios are tested: creation without optionals set, creation with optionals set ...
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.
Let's add another test case to only supply the required data - there may be some issues in the current implementation when
pagesDomain.Certificate
isnil
or even earlier whengo-gitlab
fails, becausenil
is passed togitlab.String()
- these are just suspicions though and everything might be okay as-is 👯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 removed the
Default
values and changed to testcase to include only the required ones.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.
As per my other comment, let's have multiple test cases: testing w/ and w/o optionals set.
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.
Given the subsequent
Verify import
test step both of these checks are already done, because:import
does aRead()
and therefore the Pages domain actually has to existauto_ssl_enabled
is not properly set ...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.
What is your recommendation? I would prefer to have a "create" test case, beside the import.
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.
Maybe this helps: Use ImportStateVerify TestSteps as a replacement for writing custom "CheckExists" and "CheckAttributes" check funcs.
Also see the best practice here: https://github.com/bflad/tfproviderlint/tree/main/passes/AT002#at002