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
feat: Add authentication policy resource #3098
feat: Add authentication policy resource #3098
Changes from 15 commits
e65a335
be647f8
930ec4d
56e0b51
8bb9ccb
dfe03ff
1ad1461
a2f467a
f997314
efd6d7c
71bb605
d29425d
98e47ec
384b48b
fbd053f
30ac845
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.
Please format your code with
make fmt
.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.
That is how it is auto formatted for some reason...
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.
That's interesting, but we can fix this on our side. Let's leave this unresolved.
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.
We should also support Update 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.
Are you sure, the only parameter
authentication_policy
has ForceNew enabled and with the other attachment resources it is the same.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.
Sorry, I was not clear: Can we have
authentication_policy
withoutForceNew
and handle Update in such a function? I think it should be possible.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 think that is a good example to bring up a point. I wouldn't to it at this moment, because it would introduce another inconsistency into the codebase and there are already tons of them, which becomes extremely frustrating for me as someone, which contributes only from time to time. When I run into a problem or am implementing something, I usually look at examples at another place in the codebase. However, for every given thing, there are at least 2 (usually more) options how to do it.
And at least have of the comments in this PR were "we don't do it in this way any more" or something similar, which becomes very frustrating.
Another example I am currently struggling with, is how acceptance_test are done. If I am not mistaken, there are at least three different ways. So which one shall I use, I guess is should be
Better tests poc
as with the warehouse resource (here I have the problem, that the readme has some errors and the generator makes changes to existing generated files)? Or shall I take another approach?Regarding your original request: I would do it in another PR and make changes to all attachment resources at the same time for consistency.
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.
Hi,
First of all, we really appreciate community contributions. It's great to see users' interest in this project.
The project was started by the community and taken over by Snowflake after a while. So, we expect that there will be some differences in the code. During our work before v1, we came up with a few different ideas, and we want to choose the best one ultimately, but we can't improve every resource at once.
To make contributing more straightforward, we have provided a contribution guide, and if you have any questions regarding this project, don't hesitate to ask us :)
The generator you mentioned is a PoC for speeding up writing tests. It is incomplete and has some limitations, as stated in the generator's README.
Regarding the Update thing, as I stated above, we can handle it on our side.
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.
@sfc-gh-jmichalak Sure, I am following the project for some while now while I made some small contributions. And of course, I can understand your position, that you are testing different ideas to find a way to make this project more maintainable in the future and I can perfectly understand why this project is in this state. However, this current state is quite difficult/frustrating for someone like me, which works in this project only 2-3 a year for some weeks and has to understand, what the current
best-practice
is and where to findcorrect
examples how to implement certain things.And yes, I have read the contributing guide, however, in this current state it is only partially helpful.
And btw, now from the perspective of the user: Consistent behaviour across similar resources is something what a user wishes a lot. Therefore I wouldn't recommend to add the update behaviour only to the
authentication_policy_attachment
, but to do it consistent for all attachments.And thanks for taking over the rest of the work on this resource. :)