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

Remember authentication details with endpoint configuration #1526

Merged
merged 7 commits into from
Nov 22, 2019
Merged

Remember authentication details with endpoint configuration #1526

merged 7 commits into from
Nov 22, 2019

Conversation

barmac
Copy link
Collaborator

@barmac barmac commented Oct 9, 2019 •
edited by nikku
Loading

Closes #1067

Add tests:

  • fix camunda API tests
  • fix deployment tool tests
  • fix deployment modal tests
  • save flow
  • deployment with configuration saved
  • deployment with partial configuration
  • deployment with no configuration
  • endpoint changed => single endpoint right now
  • cleanup commit history

@bpmn-io-tasks bpmn-io-tasks bot added the in progress Currently worked on label Oct 9, 2019
@barmac barmac changed the title 1067 remember authentication details with endpoint configuration Remember authentication details with endpoint configuration Oct 9, 2019
@barmac barmac force-pushed the 1067-remember-authentication-details-with-endpoint-configuration branch 3 times, most recently from 60dd5d3 to 67ae3cf Compare October 10, 2019 15:21
@barmac barmac marked this pull request as ready for review October 10, 2019 15:28
@bpmn-io-tasks bpmn-io-tasks bot added needs review Review pending and removed in progress Currently worked on labels Oct 10, 2019
@barmac

This comment has been minimized.

Sign in to view
@philippfromme

This comment has been minimized.

Sign in to view
@barmac

This comment has been minimized.

Sign in to view
@barmac barmac requested a review from nikku October 15, 2019 08:01
@barmac

This comment has been minimized.

Sign in to view
@philippfromme
Copy link
Contributor

I'm having difficulties running it on Windows.

@nikku
Copy link
Member

nikku commented Oct 15, 2019

We decided to not ship this with v3.4 of the app. Will be made available with the next version of the tool.

@nikku nikku added ready Ready to be worked on and removed needs review Review pending labels Oct 15, 2019
nikku
nikku requested changes Oct 17, 2019
View reviewed changes
Copy link
Member

@nikku nikku left a comment •
edited
Loading

Choose a reason for hiding this comment

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

As discussed, please perform the following changes:

  • remove keytar for the moment
  • add a checkbox that must be checked by the users to confirm that credentials should be stored
  • if checked, store credentials using our existing configuration mechanism

@nikku
Copy link
Member

nikku commented Oct 17, 2019

We may follow up on this PR in the future to add OS keychain support.

@barmac

This comment has been minimized.

Sign in to view
@barmac barmac force-pushed the 1067-remember-authentication-details-with-endpoint-configuration branch 2 times, most recently from 76a245d to d52f479 Compare October 21, 2019 11:43
@barmac barmac requested a review from nikku October 21, 2019 11:48
@barmac

This comment has been minimized.

Sign in to view
@nikku

This comment has been minimized.

Sign in to view
@barmac barmac force-pushed the 1067-remember-authentication-details-with-endpoint-configuration branch 2 times, most recently from ed5fdae to 29fc475 Compare October 24, 2019 13:14
@nikku nikku force-pushed the 1067-remember-authentication-details-with-endpoint-configuration branch 2 times, most recently from b2ab748 to 1380f36 Compare November 5, 2019 14:43
@nikku nikku force-pushed the 1067-remember-authentication-details-with-endpoint-configuration branch from 3a87c1d to 2e29877 Compare November 22, 2019 07:45
@nikku nikku force-pushed the 1067-remember-authentication-details-with-endpoint-configuration branch from 2e29877 to ad3c0da Compare November 22, 2019 08:30
@nikku nikku force-pushed the 1067-remember-authentication-details-with-endpoint-configuration branch from ad3c0da to 35b6873 Compare November 22, 2019 08:42
@nikku nikku added this to the M32 milestone Nov 22, 2019
@nikku nikku self-requested a review November 22, 2019 08:42
@nikku nikku force-pushed the 1067-remember-authentication-details-with-endpoint-configuration branch from 35b6873 to 5ca305d Compare November 22, 2019 09:38
@nikku
feat(deploy): remember authentication details
5334c8a 
* remember authentication details, if the user actively
  decides this
* restore global endpoint configuration, as intended until
  it is properly addressed via #804
* refactor deployment tool, pulling out validation into separate
  component
* pull out menu update functionality to be re-added at a later
  point in time, in a different place
* deployment tool UI remains untouched

Closes #1067
@nikku
chore(deployment-tool): restore keyboard shortcuts trap
d5887aa 
This is still not an optimal solution. What I'd personally
like to end up with is something that just works without
the need for modal implementors to care about it.

It is up to future work on this topic to achieve this.
@nikku
chore(deployment-tool): replace +/- details toggle with appropriate icon
e80e07b
@nikku nikku force-pushed the 1067-remember-authentication-details-with-endpoint-configuration branch from a0913ee to 9e535b6 Compare November 22, 2019 10:25
@nikku nikku requested review from pinussilvestrus and nikku and removed request for nikku and philippfromme November 22, 2019 10:41
@nikku
test(deployment-tool): bring back CamundaAPI test coverage
38bc449 
* test various untested specifics
* assert executed fetches in curial code paths (authentication)

NOTE: We still need proper assertions in some places. Right
now we at least know that all executed code paths are not throwing.
@nikku
test(deployment-tool): stub missing test
54849e8
@nikku nikku force-pushed the 1067-remember-authentication-details-with-endpoint-configuration branch from 9e535b6 to 54849e8 Compare November 22, 2019 10:53
@nikku nikku added needs review Review pending and removed ready Ready to be worked on labels Nov 22, 2019
pinussilvestrus
pinussilvestrus reviewed Nov 22, 2019
View reviewed changes
client/src/plugins/deployment-tool/CamundaAPI.js

this.baseUrl = normalizeBaseURL(endpoint.url);

this.authentication = this.getAuthentication(endpoint);
Copy link
Contributor

Choose a reason for hiding this comment

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

Really like this one, makes absolute sense 👍

pinussilvestrus
pinussilvestrus reviewed Nov 22, 2019
View reviewed changes
client/src/plugins/deployment-tool/DeploymentDetailsModal.js Outdated
componentWillUnmount() {
this.mounted = false;
componentDidMount() {
this.connectionChecker.subscribe({
Copy link
Contributor

Choose a reason for hiding this comment

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

Cool stuff, this makes the modal so much easier.

pinussilvestrus
pinussilvestrus reviewed Nov 22, 2019
View reviewed changes
client/src/plugins/deployment-tool/DeploymentTool.js
await this.saveDetails(tab, details);
configuration = await this.saveConfiguration(tab, userConfiguration);

if (action === 'save') {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be ok to keep it internally for future reference.

pinussilvestrus
pinussilvestrus approved these changes Nov 22, 2019
View reviewed changes
Copy link
Contributor

@pinussilvestrus pinussilvestrus left a comment

Choose a reason for hiding this comment

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

Very good job in the refactorings! 🚀 This will definitely help us to improve the tool in the future. I even think it improves certain situations when it comes to finally include the start instance feature (cf. ConnectionChecker). The test coverage should fit our needs for now.

One thing I could even think of, and what we discussed back on Wednesday, is to go one step further in "splitting up functionality into multiple modules" (like we did with the DeploymentConfigValidator) and think about also splitting the UI (deployment config modal opening, showing notifications, rendering toolbar actions) and the business / service logic (performing deployments) of the DeploymentTool.

For now, it works perfectly fine and nothing will hold us from creating further plugins (cf. the start instance feature), but I could think about having this as a substantial improvement in the future.

Good job at all 👍

@pinussilvestrus pinussilvestrus merged commit f7da8f1 into develop Nov 22, 2019
@bpmn-io-tasks bpmn-io-tasks bot removed the needs review Review pending label Nov 22, 2019
@delete-merged-branch delete-merged-branch bot deleted the 1067-remember-authentication-details-with-endpoint-configuration branch November 22, 2019 12:51
@pinussilvestrus pinussilvestrus mentioned this pull request Dec 9, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pinussilvestrus pinussilvestrus pinussilvestrus approved these changes

@nikku nikku Awaiting requested review from nikku

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
M32
Development

Successfully merging this pull request may close these issues.

Remember authentication details with endpoint configuration
4 participants
@barmac @philippfromme @nikku @pinussilvestrus