-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
File restructure by providers for add_cloud_metadata #4088
File restructure by providers for add_cloud_metadata #4088
Conversation
Jenkins standing by to test this. If you aren't a maintainer, you can ignore this comment. Someone with commit access, please review this and clear it for Jenkins to run. |
1 similar comment
Jenkins standing by to test this. If you aren't a maintainer, you can ignore this comment. Someone with commit access, please review this and clear it for Jenkins to run. |
@@ -0,0 +1,24 @@ | |||
package actions |
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.
Not related to this change, but why is this package called actions
? @andrewkroh
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.
At one point there was an actions folder and I think we decided in the original PR for this processor to move it into its own folder. Looks like I did the file move, but never updated the actual package name.
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.
Should I make some related changes by the way?
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 would be great. Thanks
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.
Change is made, also applied on add_locale
package.
BTW, could the reviewdog be taught to recognize such convention?
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.
Thanks.
For reviewdog, @7AC perhaps knows more.
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.
Reviewdog just does what golint
does
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.
LGTM. @andrewkroh Can you also take a look?
@@ -0,0 +1,24 @@ | |||
package actions |
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.
At one point there was an actions folder and I think we decided in the original PR for this processor to move it into its own folder. Looks like I did the file move, but never updated the actual package name.
d04d9fb
to
66e7cf1
Compare
should follow the name of their current folder
jenkins, test it |
This PR is a follow up issue for #4023 , focus on adjusting the files structure according the cloud providers. makes it easier to spot which parts are specific to a provider and which is the shared code.