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

Fix legacy 1.7 build [with potential downsides] #1401

Closed
wants to merge 6 commits into from

Conversation

allgreed
Copy link

There was some incompatibilities that prevented the build from
completing, I've patched them to make it work, commenting out a few
lines of code (marked in README) - this is likely break GCE and Azure
potentially, but I haven't tested. Works on existing (pre 2.0) nixops
project out of the box (with DigitalOcean as the cloud of choice).

But I guess that partially working legacy nixops is better than one that cannot be built

There was some incompatibilities that prevented the build from
completing, I've patched them to make it work, commenting out a few
lines of code (marked in README) - this is likely break GCE and Azure
potentially, but I haven't tested. Works on existing (pre 2.0) nixops
project out of the box (with DigitalOcean as the cloud of choice).
nix/azure.nix Outdated
type = with types; loaOf (submodule fileSystemsOptions);
};
#fileSystems = mkOption {
# type = with types; loaOf (submodule fileSystemsOptions);
Copy link
Member

Choose a reason for hiding this comment

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

loaOf is an alias to attrsOf now, so why not just change the type to attrsOf proper and be done with it? Doesn't seem necessary to comment it out and break things.

nix/gce.nix Outdated
type = with types; loaOf (submodule fileSystemsOptions);
};
#fileSystems = mkOption {
# type = with types; loaOf (submodule fileSystemsOptions);
Copy link
Member

Choose a reason for hiding this comment

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

Same.

@allgreed
Copy link
Author

@cole-h I've changed the loaOf as per your suggestion (in the whole project), however then the entire thing breaks:

error: The option `fileSystems.<name>.device' defined in `[ path to repo ]/nixops/nix/gce.nix' does not exist.

also: any idea what to put instead of type = types.listOf types.string;? This one seems to be deprecated as well

@@ -93,7 +93,7 @@ def create(self, defn, check, allow_reboot, allow_recreate):
self.creation_token = None
break
if fs["LifeCycleState"] != "creating":
raise Exception("Elastic File System ‘{0}’ is in unexpected state ‘{1}’".format(fs["LifeCycleState"]))
raise Exception("Elastic File System ‘{0}’ is in unexpected state".format(fs["LifeCycleState"]))

Choose a reason for hiding this comment

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

This just feels wrong to me. Did a little digging and these docs suggest that the first argument could be fs["Name"] or fs["FileSystemId"], or at the very least, you should instead remove the first placeholder and change the second to be {0}, as the argument passed to format is the state, so the error would make no sense as is now.

@@ -119,7 +119,7 @@ def create(self, defn, check, allow_reboot, allow_recreate):
self.state = self.UP
break
if mt["LifeCycleState"] != "creating":
raise Exception("Elastic File System mount target ‘{0}’ is in unexpected state ‘{1}’".format(mt["LifeCycleState"]))
raise Exception("Elastic File System mount target ‘{0}’ is in unexpected state".format(mt["LifeCycleState"]))

Choose a reason for hiding this comment

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

Same here, this suggests probably want mt["MountTargetId"] this time, or again, change the message to e.g. "Elastic File System mount target is in unexpected state `{0}`" instead.

@@ -1095,7 +1095,7 @@ def restore(self, defn, backup_id, devices=[]):
continue

self.log("restoring BLOB {0} from snapshot"
.format(media_link, s_id))
.format(media_link))

Choose a reason for hiding this comment

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

I'd say you should maybe should add the format placeholder, not remove the argument here?

@celestefox
Copy link

There's not a one-size-fits all replacement, NixOS/nixpkgs#66346 has a summary of the options.

I'm keeping the filesystem options in gce and azure commented out,
because the loaOf was not the reason for build failures
As per NixOS/nixpkgs#66346 suggestion I'm going
for 'str' since it's a list of arguments and is only further processed
by Python
@allgreed
Copy link
Author

@mystfox thanks for the suggestions, all done [and Travis is now passing <3]. Previously I was just trying to see if it will work ;)

Unfortunately I have no idea how to fix the fileSystems issue with azure and gce

@adisbladis
Copy link
Member

Closing this as it's very likely stale and 1.7 is not seeing any development.

Feel free to reopen if it's still relevant.

@adisbladis adisbladis closed this Jul 7, 2021
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.

None yet

4 participants