Skip to content

feat: expose premium trial form via CLI #15054

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

Merged
merged 7 commits into from
Oct 29, 2024
Merged

feat: expose premium trial form via CLI #15054

merged 7 commits into from
Oct 29, 2024

Conversation

joobisb
Copy link
Contributor

@joobisb joobisb commented Oct 14, 2024

This PR closes #14856

@cdr-bot cdr-bot bot added the community Pull Requests and issues created by the community. label Oct 14, 2024
@matifali matifali requested review from a team, Kira-Pilot, johnstcn and dannykopping and removed request for a team and Kira-Pilot October 14, 2024 07:32
Copy link
Member

@johnstcn johnstcn 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 your contribution! From my perspective, the trial information should always come directly from user input. I don't feel comfortable with allowing trials to be specified directly via CLI environment variables, although I admit there's very little stopping an enterprising user from feeding information in via expect or similar.

@joobisb
Copy link
Contributor Author

joobisb commented Oct 15, 2024

Thanks for your contribution! From my perspective, the trial information should always come directly from user input. I don't feel comfortable with allowing trials to be specified directly via CLI environment variables, although I admit there's very little stopping an enterprising user from feeding information in via expect or similar.

@johnstcn should we remove first-user-trial as well from CLI env variables ? because if its true then other variables would also be required, or else we should make those optional

Flag: "first-user-trial",

@johnstcn
Copy link
Member

@johnstcn should we remove first-user-trial as well from CLI env variables ? because if its true then other variables would also be required, or else we should make those optional

Let's leave that as-is.

Copy link

github-actions bot commented Oct 16, 2024

All contributors have signed the CLA ✍️ ✅
Posted by the CLA Assistant Lite bot.

@joobisb
Copy link
Contributor Author

joobisb commented Oct 16, 2024

@johnstcn should we remove first-user-trial as well from CLI env variables ? because if its true then other variables would also be required, or else we should make those optional

Let's leave that as-is.

@johnstcn
I've addressed the comments and I have checked through the codebase, the only issue I see is that of the use of --first-user-trial here, since the value is set this will prompt to enter the trial details causing the workflow to fail, one option is to set it as --first-user-trial=false, would that be ok?

--first-user-trial \

@matifali
Copy link
Member

@joobisb yes set that as false.

@joobisb
Copy link
Contributor Author

joobisb commented Oct 16, 2024

@joobisb yes set that as false.

i've updated the PR

@johnstcn johnstcn changed the title feat: expose 30d trial form via CLI feat: expose trial form via CLI for 30 days Oct 16, 2024
@matifali matifali changed the title feat: expose trial form via CLI for 30 days feat: expose premium trial form via CLI Oct 16, 2024
@@ -410,7 +410,7 @@ jobs:
--first-user-username coder \
--first-user-email pr${{ env.PR_NUMBER }}@coder.com \
--first-user-password $password \
--first-user-trial \
--first-user-trial=false \
Copy link
Member

Choose a reason for hiding this comment

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

We are currently not making any use of a premium license in PR deployments. If needed, we can always upload one manually.

@matifali matifali requested a review from johnstcn October 17, 2024 09:13
@matifali
Copy link
Member

Can you rebase on main?

Copy link
Member

@johnstcn johnstcn left a comment

Choose a reason for hiding this comment

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

One thing I've noticed is that in develop.sh we set --first-user-trial=true. This breaks the develop.sh workflow slightly as we then prompt for trial info. How about we instead just set --first-user-trial=false? In a separate PR we can modify the develop.sh script to upload a development license from a well-known location.

@joobisb
Copy link
Contributor Author

joobisb commented Oct 17, 2024

One thing I've noticed is that in develop.sh we set --first-user-trial=true. This breaks the develop.sh workflow slightly as we then prompt for trial info. How about we instead just set --first-user-trial=false? In a separate PR we can modify the develop.sh script to upload a development license from a well-known location.

sure, I thought since it's used in the dev environment, we can always enter values into the prompt. But you are right, to make it more seamless I think we could set it to false.

@joobisb
Copy link
Contributor Author

joobisb commented Oct 17, 2024

One thing I've noticed is that in develop.sh we set --first-user-trial=true. This breaks the develop.sh workflow slightly as we then prompt for trial info. How about we instead just set --first-user-trial=false? In a separate PR we can modify the develop.sh script to upload a development license from a well-known location.

sure, I thought since it's used in the dev environment, we can always enter values into the prompt. But you are right, to make it more seamless I think we could set it to false.

updated the PR

@joobisb
Copy link
Contributor Author

joobisb commented Oct 18, 2024

@matifali @johnstcn can we merge this ?

@johnstcn
Copy link
Member

johnstcn commented Oct 21, 2024

@matifali it looks like the e2e-premium tests are failing.
@joobisb Can you try rebasing your PR on latest main?

@joobisb
Copy link
Contributor Author

joobisb commented Oct 21, 2024

@matifali it looks like the e2e-premium tests are failing. @joobisb Can you try rebasing your PR on latest main?

@johnstcn done

@joobisb
Copy link
Contributor Author

joobisb commented Oct 24, 2024

@matifali can we merge this ?

@matifali matifali enabled auto-merge (squash) October 29, 2024 12:13
@matifali matifali disabled auto-merge October 29, 2024 12:13
@matifali matifali self-requested a review October 29, 2024 12:13
@matifali
Copy link
Member

@sreya, we are blocked here as the secret is unavailable for forks. This is a required test so can not merge without it being passed.

@sreya sreya enabled auto-merge (squash) October 29, 2024 13:02
@sreya sreya disabled auto-merge October 29, 2024 13:02
@sreya sreya merged commit 7982ad7 into coder:main Oct 29, 2024
25 of 29 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Oct 29, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
community Pull Requests and issues created by the community.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Expose 30d trial form via CLI
4 participants