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

feat: allow optional port for NFS remote #3131

Merged
merged 6 commits into from Jul 5, 2018
Merged

feat: allow optional port for NFS remote #3131

merged 6 commits into from Jul 5, 2018

Conversation

badrAZ
Copy link
Contributor

@badrAZ badrAZ commented Jul 3, 2018

Fixes #2299

Screenshots

image

image

Check list

Check items when done or if not relevant

  • PR reference the relevant issue (e.g. Fixes #007)
  • if UI changes, a screenshot has been added to the PR
  • CHANGELOG:
    • enhancement/bug fix entry added
    • list of packages to release updated (${name} v${new version})
  • documentation updated

Process

  1. create a PR as soon as possible
  2. mark it as WiP: (Work in Progress) if not ready to be merged
  3. when you want a review, add a reviewer
  4. if necessary, update your PR, and re- add a reviewer

@badrAZ badrAZ self-assigned this Jul 3, 2018
@badrAZ badrAZ changed the title [WiP] feat(settings/remotes): Allow optional port for NFS remote [WiP] feat(settings/remotes): allow optional port for NFS remote Jul 3, 2018
@badrAZ badrAZ changed the title [WiP] feat(settings/remotes): allow optional port for NFS remote feat(settings/remotes): allow optional port for NFS remote Jul 3, 2018
@badrAZ badrAZ requested a review from julien-f July 3, 2018 13:35
@badrAZ badrAZ changed the title feat(settings/remotes): allow optional port for NFS remote feat(Remotes): allow optional port for NFS remote Jul 3, 2018
@badrAZ badrAZ changed the title feat(Remotes): allow optional port for NFS remote feat: allow optional port for NFS remote Jul 3, 2018
@@ -17,8 +17,11 @@ export const parse = string => {
object.path = `/${trimStart(rest, '/')}` // the leading slash has been forgotten on client side first implementation
} else if (type === 'nfs') {
object.type = 'nfs'
const [host, path] = rest.split(':')
const [host, port, path = port] = rest.split(':')
Copy link
Member

Choose a reason for hiding this comment

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

IMHO, a regex would be safer and easier to use.

username,
password,
domain,
}) => {
Copy link
Member

Choose a reason for hiding this comment

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

Add tests related to these changes.

@@ -17,8 +17,9 @@ export const parse = string => {
object.path = `/${trimStart(rest, '/')}` // the leading slash has been forgotten on client side first implementation
} else if (type === 'nfs') {
object.type = 'nfs'
const [host, path] = rest.split(':')
const [, host, port, path] = /^([^:]+):(\d+)?:?([^:]+)$/.exec(rest)
Copy link
Member

Choose a reason for hiding this comment

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

/^([^:]+):(?:(\d+):)?([^:]+)$/

And move it in a global constant.

path: '/media/nfs',
},
},
})

const NFS_WITH_PORT_DATA = {
Copy link
Member

Choose a reason for hiding this comment

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

Move it directly in data.

@julien-f julien-f requested a review from pdonias July 4, 2018 13:07
<br />
<input
className='form-control'
ref='port'
Copy link
Member

Choose a reason for hiding this comment

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

Please rebase and make it controlled :)

@@ -3,6 +3,8 @@ import map from 'lodash/map'
import trim from 'lodash/trim'
import trimStart from 'lodash/trimStart'

const URL_RE = /^([^:]+):(?:(\d+):)?([^:]+)$/
Copy link
Member

Choose a reason for hiding this comment

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

NFS_RE?

@julien-f julien-f merged commit fb1bf6a into master Jul 5, 2018
@julien-f julien-f deleted the nfsOptionalPort branch July 5, 2018 09:09
julien-f pushed a commit that referenced this pull request Jul 5, 2018
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.

Minor backup improvements
3 participants