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

Code cleanup: Puppet logs for esx software update to follow new code convention #139

Closed
wants to merge 1 commit into from

Conversation

kaushalgala
Copy link

Changing strings in accordance with the style for new code to follow %s way of strings

Also added some timestamps for some of the strings, since Puppet logging
does not do timestamps.

…convention

Changing strings in accordance with the style for new code to follow %s way of strings

Also added some timestamps for some of the strings, since Puppet logging
does not do timestamps.
@kaushalgala
Copy link
Author

@gavin-scott did some string cleanups. Not sure which Jira to attach to. Did some end-end update tests with this PR and seems fine.

@kaushalgala
Copy link
Author

@ripienaar will be happy with this changes, i assume :)

@ripienaar
Copy link

very nice, thank you :)

@ripienaar
Copy link

Looks good to me, I am not sure how we are for timing around releases but I think it's good to merge now, @gavin-scott ?

@kaushalgala
Copy link
Author

Talked with @gavin-scott offline, we will put this in 8.3.1 timeframe

@gavin-scott
Copy link

I'd like to hold this until we branch for 8.3.1 Changes look good but most of this code isn't unit tested and I don't think we can be guaranteed there's not some kind of syntax error hiding in these changes.

@kaushalgala
Copy link
Author

@gavin-scott I can see your point and its ok if this gets merged later. However, as you can see from the code changes, we can be assured there is no syntax error hiding :). I tested the changes with actual software update workflow which covers all the workflow except the uninstall part. The uninstall part was tested by a test manifest.

btw, I noticed we don't have Travis CI integration for the unit tests for vmware-vcenter project. Any plans in future to have that ?

@gavin-scott
Copy link

We're at the stage of 8.3 where we should only be merging stop-ship fixes.

I thought Travis CI was on this. I turned it on now, if you push an update to this branch we can see if it kicks off the tests.

@kaushalgala
Copy link
Author

Is there a way to manually kick off Travis CI on desired PR, or even a commit ? That way a developer can do a pre-checkin test before submitting a PR :)

@gavin-scott
Copy link

I think if you force push to this branch it will kick it off

@kaushalgala
Copy link
Author

That I know :) I was trying to see if we can do it from the github UI itself.

@gavin-scott
Copy link

You can once it starts running tests for a PR. At that point there will be a link on the PR to Travis CI and you can start new ones. But right now Travis CI doesn't know about the PR -- it should be waiting now for new PRs or commits to run the tests.

@kaushalgala
Copy link
Author

@gavin-scott closing this PR for now since I may need to do any fixes for updates, and that would stale this PR. I will re-submit a new one for this cleanup once the branch is open for 8.3.1 / 8.4 changes.

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.

3 participants