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

make domain optional #1803

Closed
wants to merge 1 commit into from
Closed

Conversation

pmeier
Copy link
Member

@pmeier pmeier commented May 11, 2023

Reference Issues or PRs

Closes #1707.

What does this implement/fix?

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds a feature)
  • Breaking change (fix or feature that would cause existing features not to work as expected)
  • Documentation Update
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no API changes)
  • Build related changes
  • Other (please describe):

Testing

  • Did you test the pull request locally?
  • Did you add new tests?

Any other comments?

nebari init (with or without --guided-init) and nebari deploy work fine. However we get problems for everything else since we never write back the IP of the load balancer that we find during deploy. I think we have two options here:

  1. Write back the IP back. This probably a bad idea, since a deploy would potentially change the config. Since this should only happen for local deployments, it isn't too bad though.
  2. Make an empty domain field a sentinel. During the verification stage, we could inspect the cluster, if available, and extract the domain from there.

"\n\n 🪴 Great! Now you need to provide a valid domain name (i.e. the URL) to access your Nebri instance. "
"This should be a domain that you own.\n\n"
"\n\n 🪴 Great! Now you can provide a valid domain name (i.e. the URL) to access your Nebari instance. "
"This should be a domain that you own. FIXME Default is the IP of the load balancer\n\n"
Copy link
Member Author

Choose a reason for hiding this comment

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

Need to put something else here.

Comment on lines +382 to +384

print("#" * 60)
print(repr(inputs.domain_name))
Copy link
Member Author

Choose a reason for hiding this comment

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

Oops

Suggested change
print("#" * 60)
print(repr(inputs.domain_name))

Comment on lines +554 to +555
print(inputs.dict())

Copy link
Member Author

Choose a reason for hiding this comment

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

Oops

Suggested change
print(inputs.dict())

@pmeier pmeier added the needs: discussion 💬 Needs discussion with the rest of the team label May 15, 2023
@iameskild
Copy link
Member

Hey @pmeier, what is the status of this PR?

@viniciusdc
Copy link
Contributor

viniciusdc commented Jun 20, 2023

Hi @pmeier, regarding what we discussed during the last community meeting, for the destroy part, if you would like the load balancer address (or any information from the resources) to be exposed we can follow a similar approach that was done for the address test during the deploy, right now, we are getting the LB IP from terraform here we then use this function to catch all the outputs (that terraform saves under a json file), so that its consumed here under the stage_output objects.

Now, for destroy, we have this function gather_stage_outputs that will refresh all resources, and grab the needed outputs (including the IP mentioned), so you could use it in the destroy script as well in a similar fashion as the test function discussed above.

@viniciusdc
Copy link
Contributor

Also, it might be interesting to run the Kubernetes CI on this PR as well, so that we can see the error message as well. I am a bit curious to see where terraform might be complaining about the address, as this should only be failing in the schema validation

@pmeier pmeier added status: in progress 🏗 This task is currently being worked on and removed needs: discussion 💬 Needs discussion with the rest of the team labels Jun 20, 2023
costrouc added a commit that referenced this pull request Jun 21, 2023
@costrouc
Copy link
Member

Closing since implemented in #1833

@costrouc costrouc closed this Jun 21, 2023
costrouc added a commit that referenced this pull request Jun 23, 2023
costrouc added a commit that referenced this pull request Jun 28, 2023
costrouc added a commit that referenced this pull request Jul 19, 2023
costrouc added a commit that referenced this pull request Aug 2, 2023
iameskild pushed a commit that referenced this pull request Aug 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: in progress 🏗 This task is currently being worked on
Projects
Development

Successfully merging this pull request may close these issues.

[BUG] - local deploy cannot guaranteed be done without having access to a domain
4 participants