Skip to content

Conversation

@andrewjstone
Copy link
Contributor

thing-flinger is a remote deployment system for installing Omicron. It's
intended to be used to simplify and shorten the testing cycle.

More information is found in the documentation.

`thing-flinger` is a remote deployment system for installing Omicron. It's
intended to be used to simplify and shorten the testing cycle.

More information is found in the [documentation](package/src/README.adoc).
if release {
release_flag = "--release";
}
let cmd_path = "./target/debug/omicron-package";
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same thing here about absolute/relative paths. Might be safer and easier to understand to use absolute, for an automated tool like this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For this one in particular, it's how omicron package works. It relies on files in the top level omicron-directory by default. I don't really think it's particularly problematic here, as we cd to omicron_path first which is checked to be an absolute path in the newly created (post your review) validate function.

Copy link
Collaborator

@bnaecker bnaecker left a comment

Choose a reason for hiding this comment

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

Nice work here, this looks very useful! I've a few questions/comments about clarity in the docs, and also some questions about how paths work in the tool itself. I think it's worth sticking with absolute paths wherever possible.

# Location where files to install will be placed before running
# `omicron-package install`
#
# Since usernames may vary per server, this directory is relative to the user's
Copy link
Contributor

Choose a reason for hiding this comment

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

Does that mean if I put in /opt/oxide/staging/ it'll be interpreted as $HOME//opt/oxide/staging?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed it to an absolute path. I think I did it this way originally because I was concerned about using $HOME in absolute paths. I just changed this and tested it and it works.

I also added validation to check for $HOME explicitly so absolute path checking works even when it doesn't begin with a /

Copy link
Contributor

@luqmana luqmana left a comment

Choose a reason for hiding this comment

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

Thanks for doing this Andrew! As we start to test more multi-sled scenarios we're definitely going to need more tooling along these lines. I left a couple of small comments but this is great!

Copy link
Contributor Author

@andrewjstone andrewjstone left a comment

Choose a reason for hiding this comment

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

Thanks for the review @luqmana!
I addressed all your comments except the .gitignore for now.

# Location where files to install will be placed before running
# `omicron-package install`
#
# Since usernames may vary per server, this directory is relative to the user's
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed it to an absolute path. I think I did it this way originally because I was concerned about using $HOME in absolute paths. I just changed this and tested it and it works.

I also added validation to check for $HOME explicitly so absolute path checking works even when it doesn't begin with a /

@andrewjstone andrewjstone merged commit 2e5c819 into main Jan 14, 2022
@andrewjstone andrewjstone deleted the thing-flinger2 branch January 14, 2022 23:58
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