Skip to content

CLOUDSTACK-10382: Fix cloud-get-vm-data-configdrive.in - #2688

Merged
yadvr merged 1 commit into
apache:4.11from
nuagenetworks:bugfix/CLOUDSTACK-10382
Jun 7, 2018
Merged

CLOUDSTACK-10382: Fix cloud-get-vm-data-configdrive.in#2688
yadvr merged 1 commit into
apache:4.11from
nuagenetworks:bugfix/CLOUDSTACK-10382

Conversation

@fmaximus

@fmaximus fmaximus commented Jun 4, 2018

Copy link
Copy Markdown
Contributor

Description

Fix file locations and lookup.
Cleanup of code - requires bash v4.

Types of changes

  • Breaking change (fix or feature that would cause existing functionality to change)
  • New feature (non-breaking change which adds functionality)
  • Bug fix (non-breaking change which fixes an issue)
  • Enhancement (improves an existing feature and functionality)
  • Cleanup (Code refactoring and cleanup, that may add test cases)

GitHub Issue/PRs

Screenshots (if appropriate):

How Has This Been Tested?

Checklist:

  • I have read the CONTRIBUTING document.
  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
    Testing
  • I have added tests to cover my changes.
  • All relevant new and existing integration tests have passed.
  • A full integration testsuite with all test that can run on my environment has passed.

@fmaximus fmaximus added this to the 4.11.1.0 milestone Jun 4, 2018
@fmaximus fmaximus self-assigned this Jun 4, 2018
yadvr
yadvr reviewed Jun 5, 2018
View reviewed changes
Comment thread setup/bindir/cloud-get-vm-data-configdrive.in
filepath=$mountdir/cloudstack

user_data=$filepath/userdata/user_data.txt
availability_zone=$filepath/metadata/availability_zone.txt

@yadvr yadvr Jun 5, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This assumes that bash is available, will the syntactic changes work with sh?

@fmaximus fmaximus Jun 5, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The original script was only working under bash as well. function is a keyword unknown in sh.
As specified in the description, the changes require Bash 4.
I can also add a version specifically for bash 3, but I suppose with shellshock most people will have upgraded to bash 4 by now.

@DaanHoogland

DaanHoogland commented Jun 5, 2018

Copy link
Copy Markdown
Contributor

@PaulAngus I am fine with this as well. @rhtyd this is only a problem for client instances that use older versions of bash, isn't it?

@yadvr

yadvr commented Jun 5, 2018

Copy link
Copy Markdown
Member

I've not tested, but I'm okay with the change as well.

yadvr
yadvr approved these changes Jun 6, 2018
View reviewed changes

@yadvr yadvr left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM, a manual test is needed to validate the change. Integration tests are not necessary.

DaanHoogland
DaanHoogland approved these changes Jun 6, 2018
View reviewed changes

@DaanHoogland DaanHoogland left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@fmaximus' test is good enough for me.

@yadvr

yadvr commented Jun 7, 2018

Copy link
Copy Markdown
Member

LGTM, tested the script on a config drive + L2:
screenshot from 2018-06-07 11-17-40

@yadvr
yadvr merged commit 3af54ec into apache:4.11 Jun 7, 2018
winterhazel pushed a commit that referenced this pull request Jan 28, 2026
SadiJr
Merge branch 'tag-as-rule-add-variables' into '4.20.0.0-scclouds'
5d9261e 
Adição das variáveis de conta e domínio para _tags_ flexíveis

Closes #2086, #2688, and #3140

See merge request scclouds/scclouds!918
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@yadvr yadvr yadvr approved these changes
@DaanHoogland DaanHoogland DaanHoogland approved these changes
@rafaelweingartner rafaelweingartner Awaiting requested review from rafaelweingartner

Assignees

@fmaximus fmaximus

Labels

type:bug

Projects

None yet

Milestone

4.11.1.0

Development

Successfully merging this pull request may close these issues.

3 participants

@fmaximus @DaanHoogland @yadvr