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.

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

ddns-scripts: add colon char in DNS_CHARSET #25056

Closed
wants to merge 1 commit into from

Conversation

xxxxxliil
Copy link
Contributor

@xxxxxliil xxxxxliil commented Sep 29, 2024

Maintainer: @feckert
Compile tested: N/A. (Runtime package script only)
Run tested: (amd64, ibm comp PC, OpenWrt 23.04, tests done)

Description:
IPv6 is separated by : instead of ., so we need to add : in DNS_CHARSET to fix issue #25051 or #25051

Signed-off-by: Xiaolong Zhang [email protected]

@xxxxxliil xxxxxliil force-pushed the ipv6-dns-colon-char branch 2 times, most recently from 4e72f23 to 9d8a67f Compare October 1, 2024 21:16
@feckert
Copy link
Member

feckert commented Oct 7, 2024

  • Please bump the PKG_RELEASE by one.
  • Please add a valid Signed-off-by with your correct name.
  • Please add valid commit messages why we need to add ':' to the CHARSET so we could see in Git why we have to add this. A reference to the issue is not enough

@1715173329
Copy link
Member

Please add a space between your last name and first name, thanks.

@xxxxxliil
Copy link
Contributor Author

Please add a space between your last name and first name, thanks.

What if there is only first name?

@xxxxxliil
Copy link
Contributor Author

@feckert @1715173329 What else do I need to do?

@feckert
Copy link
Member

feckert commented Oct 15, 2024

I do not speak Chinese! But deepl.com says '行旅途' means 'travel destination'.
And I don't know how the names are constructed in china.
CI/CD still complains
These are the rules sorry

@xxxxxliil
Copy link
Contributor Author

CI/CD still complains
These are the rules sorry

How do I change to fit the rules?

@1715173329
Copy link
Member

Sign DCO with your legal name, at least, looks legal.
And nowadays I don't think any legal Chinese name does not contain last name.
I guess "行旅途" means "go travel".

@xxxxxliil
Copy link
Contributor Author

Sign DCO with your legal name, at least, looks legal.

Can I only sign with my legal name? I would prefer to contribute anonymously.

@1715173329
Copy link
Member

These are the rules sorry

@xxxxxliil
Copy link
Contributor Author

These are the rules sorry

sad

@xxxxxliil
Copy link
Contributor Author

@feckert @1715173329 If you have time please approve the inspection.

@feckert
Copy link
Member

feckert commented Nov 6, 2024

Looks good. Just a little nit pick, then we can merge. Can you please limit the commit message to 80 characters per line and remove the second mention of #25051. This does not need to be written twice in the commit message.

@xxxxxliil
Copy link
Contributor Author

...and remove the second mention of #25051. This does not need to be written twice in the commit message.

If you look in git, it's actually a github issue link and the string #25051. I'll keep only one if you ask me to keep only one though.

@feckert
Copy link
Member

feckert commented Nov 6, 2024

Using git cmd on the shell I see:

IPv6 is separated by `:` instead of `.`, so we need to add `:` in DNS_CHARSET to fix issue #25051
or https://github.com/openwrt/packages/issues/25051

On the github WebUI I see:

IPv6 is separated by `:` instead of `.`, so we need to add `:` in DNS_CHARSET to fix issue #25051
or #25051

Please write https://github.com/openwrt/packages/issues/25051 so no github user also can see the link. On the github WebUI the link is auto resolved. Therefore #25051 appears twice.

@feckert
Copy link
Member

feckert commented Nov 6, 2024

Also I see in my git history:
2024-09-30 03:46 +0800 行旅途 o [pr/upstream-25056] ddns-scripts: add colon char in DNS_CHARSET

But in the commit message you have added the line
Signed-off-by: Xiaolong Zhang <[email protected]>

I think if I start running the CI/CD then this will not be happy and will complain Test Formalities / Test Formalities (pull_request)
You have to update your .gitconfig with the name in your Signed-off-by line so we have.

[user]
name = Xiaolong Zhang
email = [email protected]

Then add the change wit git add <file> and after that execute git commit -s. This will add a valid Signed-off-by and the correct auther to the repository so we have.

2024-09-30 03:46 +0800 Xiaolong Zhang o [pr/upstream-25056] ddns-scripts: add colon char in DNS_CHARSET

IPv6 is separated by `:` instead of `.`, so we need to add `:` in DNS_CHARSET
fix: `https://github.com/openwrt/packages/issues/25051` or openwrt#25051

Signed-off-by: Xiaolong Zhang <[email protected]>
@xxxxxliil
Copy link
Contributor Author

Also I see in my git history: 2024-09-30 03:46 +0800 行旅途 o [pr/upstream-25056] ddns-scripts: add colon char in DNS_CHARSET

But in the commit message you have added the line Signed-off-by: Xiaolong Zhang <[email protected]>

I think if I start running the CI/CD then this will not be happy and will complain Test Formalities / Test Formalities (pull_request) You have to update your .gitconfig with the name in your Signed-off-by line so we have.

[user]
name = Xiaolong Zhang
email = [email protected]

Then add the change wit git add <file> and after that execute git commit -s. This will add a valid Signed-off-by and the correct auther to the repository so we have.

2024-09-30 03:46 +0800 Xiaolong Zhang o [pr/upstream-25056] ddns-scripts: add colon char in DNS_CHARSET

The corresponding operation for submitting through GitHub codespace is to change your name in GitHub and enter git commit --author=... --amend -asS in the terminal to edit the submission again and sign it with pgp.

Anyway, I finally completed the CI/CD name requirement, but it was a bit cumbersome.

@feckert
Copy link
Member

feckert commented Nov 7, 2024

I Update the commit message und also bumped the PKG_RELEASE number and pushed directly
b962029
Thanks for your work -> merged

@feckert feckert closed this Nov 7, 2024
@xxxxxliil xxxxxliil deleted the ipv6-dns-colon-char branch November 7, 2024 11:26
@Neustradamus
Copy link

@xxxxxliil: Good job!

@feckert: I think it can be backported in 24.10.x and 23.05.x?

@feckert
Copy link
Member

feckert commented Nov 11, 2024

Already backported with commit e95262a

@Neustradamus
Copy link

@feckert: Thanks :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants