Skip to content
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.

Sign up for GitHub

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

Jump to bottom

chore: Correct typos #75

Closed
wants to merge 5 commits into from
Closed

chore: Correct typos #75

wants to merge 5 commits into from

Conversation

AccAutomaton
Copy link
Contributor

@AccAutomaton AccAutomaton commented Sep 19, 2023 •
edited
Loading

修正placeholder中的错别字。

@f2c-ci-robot
Copy link

f2c-ci-robot bot commented Sep 19, 2023

@AccAutomaton: Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@f2c-ci-robot f2c-ci-robot bot added the do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. label Sep 19, 2023
@f2c-ci-robot
Copy link

f2c-ci-robot bot commented Sep 19, 2023

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please assign longjuan after the PR has been reviewed.
You can assign the PR to them by writing /assign @longjuan in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

JohnNiang
JohnNiang requested changes Sep 19, 2023
View reviewed changes
src/main/resources/extensions/policy-template-s3os.yaml
@@ -44,14 +44,14 @@ spec:
validation: required
help: 协议头请在上方设置，此处无需以"http://"或"https://"开头，系统会自动拼接
- $formkit: password
name: accessKey
name: accessKeyId
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

不建议更改 name，这将会造成破坏性更新。所有更新插件的用户都得重新配置才能够正常工作。

如果确实需要更改 name，那么在获取 accessKeyId 的时候应当同时获取 accessKey 来保证升级后没有问题。

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

name修改已回滚，当前pr仅修改错别字。

@JohnNiang
Copy link
Member

JohnNiang commented Sep 19, 2023 •
edited
Loading

当前 PR 包含两个功能上的修改，建议分两个 PR 提交。可参考：https://docs.halo.run/contribution/pr。

@longjuan
Copy link
Member

我认为不应该直接在 setter 中替换，setter 应保留原始意义，万一后面需要获取原始的 location 的值的话就没办法了。
应写个新方法去获取经过占位变量替换后的 location 的值，然后替换原来的调用。
image

acautomaton added 2 commits September 19, 2023 15:25
@AccAutomaton
Copy link
Contributor Author

我认为不应该直接在 setter 中替换，setter 应保留原始意义，万一后面需要获取原始的 location 的值的话就没办法了。 应写个新方法去获取经过占位变量替换后的 location 的值，然后替换原来的调用。 image

此问题PR在 #74
另，如果选择修改getter而不将路径作为文件本身属性，那么当需要取用文件的时候如何知道该文件是哪一天上传的呢

@AccAutomaton AccAutomaton changed the title refactor: Optimization of variable name about access key chore: Correct typos Sep 19, 2023
@longjuan
Copy link
Member

那么当需要取用文件的时候如何知道该文件是哪一天上传的呢

Properties 只在上传时发挥作用，上传完成后会把 objectkey 写入 attachment 里，不会再次调用 getlocation。

@AccAutomaton
Copy link
Contributor Author

当前 PR 包含两个功能上的修改，建议分两个 PR 提交。可参考：https://docs.halo.run/contribution/pr。

有点知识盲区了，我确实分了两个分支和PR，结果修改却合在一起了...

@AccAutomaton
Copy link
Contributor Author

当前 PR 包含两个功能上的修改，建议分两个 PR 提交。可参考：https://docs.halo.run/contribution/pr。

有点知识盲区了，我确实分了两个分支和PR，结果修改却合在一起了...

破案了，迁出分支的时候没从main迁出，麻了， 这下宇宙大爆炸

@AccAutomaton
Copy link
Contributor Author

我会关闭这个PR，后续请移步 #74

@AccAutomaton AccAutomaton deleted the opt_variable_name_about_access_key branch September 19, 2023 07:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JohnNiang JohnNiang JohnNiang requested changes

@longjuan longjuan Awaiting requested review from longjuan

@wan92hen wan92hen Awaiting requested review from wan92hen

Assignees
No one assigned
Labels
do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@AccAutomaton @JohnNiang @longjuan