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

[16.0][ADD]base_dns_infrastructure #20

Merged
merged 2 commits into from
Jul 10, 2024
Merged

[16.0][ADD]base_dns_infrastructure #20

merged 2 commits into from
Jul 10, 2024

Conversation

flotho
Copy link
Member

@flotho flotho commented May 26, 2024

supersedes #18

@flotho flotho mentioned this pull request May 26, 2024
Closed
@flotho
Copy link
Member Author

flotho commented May 26, 2024

hi @bosd
I tried from scratch here :

  • checkout the 16.0 branch
  • create a new branch
  • copy only the module in the new branch
  • push everything

So everything should be up to date.
Sadly still struggling with the pre-commit.
Any help will be appreciated,

Regards

@flotho
Copy link
Member Author

flotho commented May 26, 2024

at last ....

@flotho
Copy link
Member Author

flotho commented May 26, 2024

ping @DorianMAG @gaelTorrecillas @JulienMartinez

DorianMAG
DorianMAG approved these changes May 31, 2024
View reviewed changes
Copy link

@DorianMAG DorianMAG left a comment

Choose a reason for hiding this comment

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

Test in runboat,
to see app, go in manger users and pass 'Infrastructure and DNS' parameter' to 'DNS user' or 'DNS manager'.
Thx for this work !
LGTM

@flotho
Copy link
Member Author

flotho commented Jun 3, 2024

hi @bosd
Any chance to have a review

@OCA-git-bot
Copy link
Contributor

This PR has the approved label and has been created more than 5 days ago. It should therefore be ready to merge by a maintainer (or a PSC member if the concerned addon has no declared maintainer). 🤖

bosd
bosd suggested changes Jun 4, 2024
View reviewed changes
Copy link

@bosd bosd left a comment

Choose a reason for hiding this comment

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

Looks good. ✨ Some bug found. Minor textual suggestions.

Nitpiking:
Some dome data would be nice. But that would be non blocking.

base_dns_infrastructure/models/dns_record/common.py Outdated Show resolved Hide resolved
base_dns_infrastructure/models/dns_record/common.py Outdated Show resolved Hide resolved
base_dns_infrastructure/views/dns_domain_zone.xml Show resolved Hide resolved
base_dns_infrastructure/models/dns_domain_zone/common.py Outdated Show resolved Hide resolved
base_dns_infrastructure/models/dns_domain_zone/common.py Outdated Show resolved Hide resolved
base_dns_infrastructure/static/description/icon.png Outdated Show resolved Hide resolved
base_dns_infrastructure/views/dns_domain_zone.xml Outdated Show resolved Hide resolved
base_dns_infrastructure/models/dns_record/common.py Outdated Show resolved Hide resolved
@flotho
Copy link
Member Author

flotho commented Jun 5, 2024

Looks good. ✨ Some bug found. Minor textual suggestions.

Nitpiking: Some dome data would be nice. But that would be non blocking.

thanks for this awesome review. I'll have a look to this ASAP

@flotho
Copy link
Member Author

flotho commented Jun 5, 2024

Hi @bosd hope it's better

@flotho flotho requested a review from bosd June 5, 2024 11:32
@bosd
Copy link

bosd commented Jun 5, 2024

@flotho Thanks for your very quick reply. 👍 Sadly the bug still needs some work.

@flotho
Copy link
Member Author

flotho commented Jun 5, 2024

@flotho Thanks for your very quick reply. 👍 Sadly the bug still needs some work.

thanks for your fast review. I'll have a look to this ASAP

@flotho
Copy link
Member Author

flotho commented Jun 8, 2024

Hi @bosd , everything looks fine now

@bosd
Copy link

bosd commented Jun 8, 2024

@flotho
Thanks for all your hard work on this module.

Functionally & Technically it is ok for me now.
(except naming thingy 😉 )

The manifest and readme list other contributors. But reading from the commits your the only author.
(If some code is re-used some purists might want to see their commits in the history as well.)
I'm gonne withhold from that discussion. I don' t have knowledge of the history of this module. And what the oca want us to do in these cases.
So I'm gonna treat it as a new addition 😉 .

So, now let's make this thing ready to get merged ✨
Please squash all your commits.
So with 2 approving reviews, it can be merged.

Thanks for all your work on this.
Gonna approve now.

@OCA-git-bot
Copy link
Contributor

This PR has the approved label and has been created more than 5 days ago. It should therefore be ready to merge by a maintainer (or a PSC member if the concerned addon has no declared maintainer). 🤖

DorianMAG
DorianMAG approved these changes Jun 11, 2024
View reviewed changes
Copy link

@DorianMAG DorianMAG left a comment

Choose a reason for hiding this comment

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

Tested in runboat
LGTM

@flotho
[16.0][ADD]base_dns_infrastructure
3781204 
[LINT]Run the precommit

[16.0][ADD]base_dns_infrastructure

[FIX]base_dns_infrastructure: bad message

[IMP]base_dns_infrastructure: Demo data

[IMP]base_dns_infrastructure: Demo data + Security
@flotho
[IMP]base_dns_infrastructure: Pylint
48ab63f 
[IMP]base_dns_infrastructure: Pylint
@flotho
Copy link
Member Author

flotho commented Jun 11, 2024

@flotho Thanks for all your hard work on this module.

Functionally & Technically it is ok for me now. (except naming thingy 😉 )

The manifest and readme list other contributors. But reading from the commits your the only author. (If some code is re-used some purists might want to see their commits in the history as well.) I'm gonne withhold from that discussion. I don' t have knowledge of the history of this module. And what the oca want us to do in these cases. So I'm gonna treat it as a new addition 😉 .

So, now let's make this thing ready to get merged ✨ Please squash all your commits. So with 2 approving reviews, it can be merged.

Thanks for all your work on this. Gonna approve now.

done !

DorianMAG
DorianMAG approved these changes Jun 12, 2024
View reviewed changes
Copy link

@DorianMAG DorianMAG left a comment

Choose a reason for hiding this comment

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

Tested in runboat
LGTM

@flotho
Copy link
Member Author

flotho commented Jun 26, 2024 •
edited
Loading

ping @OCA/infrastructure-maintainer , any chance to have a merge here ?
or maybe @elicoidal or @pedrobaeza ?

@flotho
Copy link
Member Author

flotho commented Jul 9, 2024

Good evening @pedrobaeza @elicoidal , sorry for buzzing
It seems that there is no maintainers in this repo and I'm wondring how to get a merge on this one ?

Regards

@pedrobaeza pedrobaeza added this to the 16.0 milestone Jul 10, 2024
@pedrobaeza
Copy link
Member

/ocabot merge nobump

@OCA-git-bot
Copy link
Contributor

Hey, thanks for contributing! Proceeding to merge this for you.
Prepared branch 16.0-ocabot-merge-pr-20-by-pedrobaeza-bump-nobump, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit 8afd912 into OCA:16.0 Jul 10, 2024
7 checks passed
@OCA-git-bot
Copy link
Contributor

Congratulations, your PR was merged at bbf18eb. Thanks a lot for contributing to OCA. ❤️

@flotho
Copy link
Member Author

flotho commented Jul 10, 2024

/ocabot merge nobump

thank you @pedrobaeza
BTW, I proposed myself as a member of the infrastructure dns team . I send an email on community group. Is it the proper process ?
Regards

@pedrobaeza
Copy link
Member

Propose it here: https://github.com/OCA/repo-maintainer-conf

@flotho
Copy link
Member Author

flotho commented Jul 10, 2024

Propose it here: https://github.com/OCA/repo-maintainer-conf

(bow)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bosd bosd bosd approved these changes

@gaelTorrecillas gaelTorrecillas gaelTorrecillas approved these changes

@DorianMAG DorianMAG DorianMAG approved these changes

Assignees
No one assigned
Labels
approved merged 🎉 ready to merge
Projects
None yet
Milestone
16.0
Development

Successfully merging this pull request may close these issues.
6 participants
@flotho @OCA-git-bot @bosd @pedrobaeza @gaelTorrecillas @DorianMAG