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 vpn config to wizard #100

Merged
merged 52 commits into from
Aug 27, 2017
Merged

add vpn config to wizard #100

merged 52 commits into from
Aug 27, 2017

Conversation

andibraeu
Copy link
Member

No description provided.

@andrenarchy andrenarchy self-requested a review August 20, 2017 18:54
Copy link
Member

@andrenarchy andrenarchy left a comment

Choose a reason for hiding this comment

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

There are a few minor things in the comments.

The most important thing is that we should decide how to proceed with the mesh tunnel. It doesn't fit where it is right now. Any ideas?

{
fileExtensions: '.ovpn,.cnf,text/*',
property: 'config',
tunnelType: 'internettunnel',
Copy link
Member

Choose a reason for hiding this comment

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

This is repeated in every element here so we should remove it. If we really need it for the ids then we can pass it as an extra binding to the wizard-upload-file component.

Copy link
Member Author

Choose a reason for hiding this comment

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

done

{
fileExtensions: '.crt,text/*',
tunnelType: 'internettunnel',
property: 'cacert',
Copy link
Member

Choose a reason for hiding this comment

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

this should be ca

Copy link
Member Author

Choose a reason for hiding this comment

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

done

{
fileExtensions: '.key,text/*',
tunnelType: 'internettunnel',
property: 'takey',
Copy link
Member

Choose a reason for hiding this comment

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

what is this?

Copy link
Member Author

Choose a reason for hiding this comment

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

tls auth key

Copy link
Member

Choose a reason for hiding this comment

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

Ah, this is called tls-auth. We have to match the openvpn option names here, there is no additional translation of these options in the backend.

Copy link
Member Author

Choose a reason for hiding this comment

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

done

constructor($scope) {
'ngInject';

this.newInternet = {};
this.internetTunnelEnabled = false;
this.meshTunnelEnabled = false;
this.internetTunnelConf = [
Copy link
Member

Choose a reason for hiding this comment

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

We could call this tunnelFiles because it's independent of the tunnelType if we decide to remove this (see below)

Copy link
Member Author

Choose a reason for hiding this comment

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

done

},
];

this.internetTunnelConfSecret = [
Copy link
Member

Choose a reason for hiding this comment

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

why are these separate? Can't we simply include them above?

Copy link
Member Author

Choose a reason for hiding this comment

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

it is for display reasons, to seperate it and show a help text in between

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, we don't have a help text for the others either. The configs can be arbitrarily complex and we can't document all of them. We could explain the most common combinations in the proposed wiki page, what do you think?

Copy link
Member Author

Choose a reason for hiding this comment

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

who will write the wiki page? :)

Copy link
Member Author

Choose a reason for hiding this comment

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

I added #102 to solve this

<div ng-click="toggleVpnList()">
<span ng-show="!state.internet.showVpnList">&#9658;</span>
<span ng-show="state.internet.showVpnList">&#9660;</span>
<div ng-click="$ctrl.toggleVpnList()">
Copy link
Member

Choose a reason for hiding this comment

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

I think we should just link to a wiki page and remove the list from the wizard. The wiki page can also go into more detail.

Copy link
Member Author

Choose a reason for hiding this comment

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

I created #101 to solve this

constructor($scope) {
'ngInject';

this.newInternet = {};
this.internetTunnelEnabled = false;
this.meshTunnelEnabled = false;
Copy link
Member

Choose a reason for hiding this comment

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

it's not wrong to initialize these here but we don't need to. They're undefined by default and updateFromInput() will set them anyway.

Copy link
Member Author

@andibraeu andibraeu Aug 21, 2017

Choose a reason for hiding this comment

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

stupid javascript ;)

but done

@@ -8,17 +8,128 @@ export default module('app.components.wizard-internet', [])
},
// TODO: handle vpn
Copy link
Member

Choose a reason for hiding this comment

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

remove this comment :)

Copy link
Member Author

Choose a reason for hiding this comment

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

done

accept="{{$ctrl.details.fileExtensions}}"
base64="true"
on-loaded="$ctrl.uploadContent(content)"
required="{{$ctrl.details.required}}">
Copy link
Member

Choose a reason for hiding this comment

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

without {{}} (unless you really want a string here which I doubt)

Copy link
Member Author

Choose a reason for hiding this comment

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

in fact we need a string here, for conditions like !ctrl.internet.share

Copy link
Member

Choose a reason for hiding this comment

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

I don't understand this one. Can you explain?

Copy link
Member Author

Choose a reason for hiding this comment

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

you're right, it would be just true or false... done


setMeshTunnelType(type) {
if (!this.newInternet.meshTunnel) {
this.newInternet.meshTunnel = {};
Copy link
Member

Choose a reason for hiding this comment

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

It's still a bit weird to have the mesh tunnel in the Internet sharing section and in the internet object. Any idea where we should put it? We can also create a new section for this or for mesh-related things in general. Could also be an advanced feature? Any opinions?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure. On the other hand, mesh vpn doesn't make sense without internet sharing. Should we discuss it in a separate issue?

Copy link
Member

Choose a reason for hiding this comment

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

I think it does make sense without internet sharing. But yeah, let's discuss it in a separate issue. What do you think about removing mesh tunnel for now from the UI?

Copy link
Member Author

Choose a reason for hiding this comment

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

Why do we need mesh vpn when we don't have internet access?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'd like to let it here for now, so we don't forget it later

Copy link
Member Author

Choose a reason for hiding this comment

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

opened #103 for this

Copy link
Member

Choose a reason for hiding this comment

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

Share internet is not about having internet access but about sharing your internet connection. Even if you don't want to share it your router may be connected to the internet and you may want to connect with other mesh nodes via a tunnel, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

sounds like an edge case :)

I'd like to let it here for now, until we have another solution, i.e. something for advanced or mesh settings, as I think this feature should be supported

import {copy, fromJson, module} from 'angular';

export default module('app.components.wizard-upload-configfile', [])
.component('wizardUploadConfigfile', {
Copy link
Member

Choose a reason for hiding this comment

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

This component looks fairly generic now and maybe we also want to use it outside of the wizard or for other than config files. What do you think about renaming it to load-file-form (it basically wraps load-file-button for use in a form)?

Copy link
Member Author

Choose a reason for hiding this comment

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

done

@andrenarchy
Copy link
Member

@andibraeu can you test this one last time please? I did some refactoring and fixed the data flow (had non-terminating angular digest cycles)... And I didn't change any of the DE translations yet.

@andibraeu
Copy link
Member Author

andibraeu commented Aug 27, 2017 via email

@andrenarchy
Copy link
Member

You can clear files individually now so we don't need the clear all button anymore. Did you use Firefox? Will check this in ~30min.

@andibraeu
Copy link
Member Author

I used Chromium

@andrenarchy
Copy link
Member

Does it work with mesh tunnel?

@andrenarchy
Copy link
Member

I can reproduce it here. Will work on it later. :)

@andrenarchy
Copy link
Member

Fixed. @andibraeu can you verify?

@andibraeu
Copy link
Member Author

not it works! do you press the button, please? Reinhold!

@andrenarchy
Copy link
Member

With pleasure :)

@andrenarchy andrenarchy merged commit 4434aee into master Aug 27, 2017
@andrenarchy andrenarchy deleted the vpn-config branch August 27, 2017 13:50
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.

2 participants