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

[ADD] connector_dns #4

Merged
merged 7 commits into from
Feb 5, 2017
Merged

[ADD] connector_dns #4

merged 7 commits into from
Feb 5, 2017

Conversation

noahzaozao
Copy link

No description provided.

@noahzaozao
Copy link
Author

@elicoidal I've moved connector_dns here, please reveiw this PR.


#. Go to ...

.. image:: https://odoo-community.org/website/image/ir.attachment/5784_f2813bd/datas

Choose a reason for hiding this comment

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

adapt

Copy link
Author

Choose a reason for hiding this comment

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

@elicoidal How to change this line ?

Choose a reason for hiding this comment

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

Don't worry.
I will create PR change the README once it is stable enough

@coveralls
Copy link

coveralls commented Aug 29, 2016

Coverage Status

Coverage remained the same at 56.463% when pulling d452aae on noahzaozao:8.0-add-connector-dns into 7c5661d on OCA:8.0.

'author': 'Elico Corp,Odoo Community Association (OCA)',
'license': 'AGPL-3',
'website': 'https://www.elico-corp.com',
'images': [],
Copy link

Choose a reason for hiding this comment

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

Remove empty keys

@lasley
Copy link

lasley commented Aug 29, 2016

Thanks for the submission @noahzaozao - comments inline. Also, can you please add test coverage? IMO this is going to be used as the base for a lot of things, so anything less than 80% is a 👎 for me - and honestly that's still pretty low.

FYI you may be able to copy a lot of testing logic from a connector I am working on. It has heavy coverage in areas where the other connectors are missing, like all of this core stuff. The Magento connector has some good tests too. Hopefully those help 😄

@hbrunn hbrunn added this to the 8.0 milestone Sep 26, 2016
@noahzaozao
Copy link
Author

@elicoidal Not sure how to fix the runbot warning

@elicoidal
Copy link

@lasley do you have any idea here how to get this rolling?

@lasley
Copy link

lasley commented Sep 30, 2016

Flying to Belgium today, was planning on knocking this & prelim Docker connector out on the plane.

* Require zone in record
* Fix action names in backend, zone, record
* Remove states from backend and zone
@lasley
Copy link

lasley commented Oct 5, 2016

All points attended to except for Settings/Connector menu is empty. Forgive my ignorance on this one, but what exactly should go in that menu?

noahzaozao/infrastructure-dns#4

@hbrunn
Copy link
Member

hbrunn commented Oct 8, 2016

@lasley usually, a wizard derived from ir.config.settings that allows users to configure the module. There might (I'm not very active with the connector framework) even be some base module where you can inject a checkbox to install your module from the configuration wizard

[FIX] connector_dns: Build and functional errors
@coveralls
Copy link

Coverage Status

Coverage remained the same at 91.277% when pulling 7b9c230 on noahzaozao:8.0-add-connector-dns into 7c5661d on OCA:8.0.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage remained the same at 91.277% when pulling 7b9c230 on noahzaozao:8.0-add-connector-dns into 7c5661d on OCA:8.0.

@lasley
Copy link

lasley commented Oct 10, 2016

Exactly what I needed, thanks @hbrunn

Will submit a new PR adding the config settings sometime this week

* Add empty group in connector settings for DNS
@lasley
Copy link

lasley commented Oct 11, 2016

Added blank config view in noahzaozao/infrastructure-dns#5 - there was no need for the model at the moment because no extra modules to enable.

@lasley lasley mentioned this pull request Oct 11, 2016
3 tasks
[ADD] connector_dns: DNS Connector settings view
@coveralls
Copy link

coveralls commented Oct 12, 2016

Coverage Status

Coverage remained the same at 91.277% when pulling 4c63b75 on noahzaozao:8.0-add-connector-dns into 7c5661d on OCA:8.0.

@noahzaozao
Copy link
Author

noahzaozao commented Oct 14, 2016

@elicoidal Once this PR has been merged , I can start re-factory the dependency modules such as connector_dnspod

@noahzaozao
Copy link
Author

@elicoidal Shall we move forward with this PR ?

@elicoidal
Copy link

@lasley Are we good to move forward?

@lasley
Copy link

lasley commented Jan 2, 2017

Yeah it would be nice to see this merged

@elicoidal
Copy link

@hbrunn @gurneyalex any feedback before I merge?

@hbrunn
Copy link
Member

hbrunn commented Jan 3, 2017

I didn't do any review, so I don't have anything to say about this PR (and it's too big to slip in a review right now). Better rely on people who actually looked at the code for a judgement, formally nobody approved it up to now...

@elicoidal
Copy link

@hbrunn Thanks for the quick feedback. I agree with you on both remarks :)

On my side: 👍
@lasley ?

Copy link

@lasley lasley left a comment

Choose a reason for hiding this comment

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

👍 from me on the stuff I didn't write. Obviously a 👍 on the stuff I did 😉

@flotho
Copy link
Member

flotho commented Feb 4, 2017

any chance to see this PR merge ?

@elicoidal
Copy link

I ll merge it!

@elicoidal elicoidal merged commit 7bcf645 into OCA:8.0 Feb 5, 2017
@elicoidal
Copy link

@noahzaozao @seb-elico

ruter-lyu pushed a commit to JoJoJoJoJoJoJo/infrastructure-dns that referenced this pull request Apr 2, 2019
…to-oca/l10n-china

[IMP]add module website_certificate.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants