Skip to content

Conversation

@NL66278
Copy link

@NL66278 NL66278 commented Jan 27, 2026

This module will be extended with modules to connect to http systems, with additional modules to use oauth authorization.

This code is running in production at several customer sites already, together with additional modules for which PR's will follow.

@NL66278 NL66278 force-pushed the 16.0-base_external_system-extendable branch 2 times, most recently from 4790a4f to daa8398 Compare January 27, 2026 12:40
)
% {
"exception": exc,
"system_name": self.name,

Choose a reason for hiding this comment

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

Personally I always find it slightly annoying if exceptions are converted to something else than they previously were. At the very least can we log the traceback, either in the logfile or to the chatter if it exists? Then it is possible to know where things went wrong.

Copy link
Author

Choose a reason for hiding this comment

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

Basically this is feedback to the user that he/she probably made an error configuring the connection. The action is to test the connection. So I think a ValidationError is the right thing to do here.

@@ -1,3 +1,2 @@
id,name,model_id:id,group_id:id,perm_read,perm_write,perm_create,perm_unlink
access_external_system_os_admin,access_external_system_os_admin,model_external_system_os,base.group_system,1,1,1,1

Choose a reason for hiding this comment

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

Would this line need to be there for external_system_adapter_os still?

Copy link
Author

Choose a reason for hiding this comment

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

No, as it has become an abstract class.

Copy link

@thomaspaulb thomaspaulb left a comment

Choose a reason for hiding this comment

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

some minor comments

@NL66278 NL66278 force-pushed the 16.0-base_external_system-extendable branch 4 times, most recently from f01995c to 5d9e8da Compare February 9, 2026 22:01
This module will be extended with modules to connect to http systems,
with additional modules to use oauth authorization.
@NL66278 NL66278 force-pushed the 16.0-base_external_system-extendable branch from 5d9e8da to d9aecbe Compare February 9, 2026 22:42
@NL66278
Copy link
Author

NL66278 commented Feb 9, 2026

@thomaspaulb I took up your comments, please look again

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.

3 participants