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

GUACAMOLE-926: Fix connection CSV imports for fields with "(attribute)" and "(parameter)" suffixes #952

Merged
merged 1 commit into from
Mar 5, 2024

Conversation

leonard2901
Copy link
Contributor

For a CSV header which ends with " (attribute)" or " (parameter)" header.replace(SUFFIX) is called. However, replace expects a second parameter. If it is omitted it will be undefined which results in the header string headernameundefined and the field not being imported correctly.

This can be reproduced when importing a CSV file like this:

name,protocol,username,password,hostname,group,users,groups,guacd-encryption (attribute)
importedconnection,rdp,bob,pass2,conn2.web.com,ROOT,guac user 1,,ssl

@necouchman
Copy link
Contributor

@leonard2901 Could you please re-base this against the staging/1.5.5 branch so that we can get it into the bugfix release we're working toward?

@leonard2901
Copy link
Contributor Author

The import module does not seem to be included in staging/1.5.5.
I assumed that this feature was already released. Is this not the case?
Sorry, I am not completely familiar with the branching and release process.

@necouchman
Copy link
Contributor

@leonard2901 - Ah, sorry about that, Leonard, you are correct - this is in the Git repo under the master branch, but not in the current 1.5.x release.

In that case, maybe you could just put this under the GUACAMOLE-926 issue, which is still open, and is the overall issue for implementing that feature?

@leonard2901
Copy link
Contributor Author

Sure. I just linked the Jira issues.
How should we proceed with this PR then?

@necouchman
Copy link
Contributor

@leonard2901 - My vote would be to actually change the title of this PR and the commit messages to "GUACAMOLE-926:", instead of GUACAMOLE-1925, and just close 1925 out completely and do all of the work under 926, since it's still open/unreleased.

@leonard2901 leonard2901 changed the title GUACAMOLE-1925: Fix connection CSV imports for fields with "(attribute)" and "(parameter)" suffixes GUACAMOLE-926: Fix connection CSV imports for fields with "(attribute)" and "(parameter)" suffixes Mar 4, 2024
@necouchman
Copy link
Contributor

Okay, the only other thing is that I think this should be re-based against, and the PR should be to merge into the next branch. If I understand our recent changes correctly, we will merge the PR into next and from there push to main. @mike-jumper?

@mike-jumper
Copy link
Contributor

@necouchman Nope! main is correct here. next is for things like the massive migration from AngularJS to Angular - things that would require going all the way to 2.0.0.

@mike-jumper
Copy link
Contributor

@neandrake
Copy link
Contributor

Here's a bit of a write-up: https://cwiki.apache.org/confluence/display/GUAC/Version+Numbering+and+Branching+Scheme

In think there’s a typo under the description of “minor releases”, it states

Changes that substantially affect backward compatibility are never part of major releases…

I think that is meant to be “never part of minor releases…”

@necouchman
Copy link
Contributor

Changes that substantially affect backward compatibility are never part of major releases…

I think that is meant to be “never part of minor releases…”

I think you're correct - I've updated it.

@necouchman necouchman merged commit e54069b into apache:main Mar 5, 2024
1 check passed
@leonard2901 leonard2901 deleted the GUACAMOLE-1925 branch March 5, 2024 14:34
@neandrake
Copy link
Contributor

@necouchman thank you, and thank you for writing up the process in the wiki!

@necouchman
Copy link
Contributor

@neandrake Thanks for the fix - and Mike was the one who wrote it up... :-)

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.

4 participants