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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Making theme ID required to make it harder to edit a live theme #739

Merged
merged 3 commits into from
Jul 27, 2020

Conversation

tanema
Copy link
Contributor

@tanema tanema commented Jul 23, 2020

fixes #699
fixes #670

This makes a tighter constraint on the theme ID and makes the tool a lot safer for changing live themes.

Before

  • If a theme ID was not provided or the string 'live' was provided as a theme ID, themekit would assume the live theme was to be operated on.
  • If the theme was the live theme, a warning would be displayed but continue its operation.

After

  • Theme IDs are strictly required. There is an error if the theme ID is blank.
  • There is an error raised if the tool tries changing the live theme
  • if the flag --allow-live is passed then the tool will be able to change the live theme.

Warn Checklist

I think technically this is a breaking change so it should require us to make a major version change

  • This changes the interface and requires a Major/Minor version change.
  • I have 馃帺'd these changes by using the commands I changed by hand.
  • I have added a dependancy to the project.

@tanema tanema requested a review from andyw8 July 23, 2020 19:36
@@ -28,6 +28,10 @@ var getCmd = &cobra.Command{
For more documentation please see http://shopify.github.io/themekit/commands/#get
`,
RunE: func(cmd *cobra.Command, args []string) error {
// This is a hack to get around theme ID validation for the list operation which doesnt need it
Copy link
Contributor

Choose a reason for hiding this comment

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

What would the non-hacky fix be? Would it need changes on core?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It would require making the validations of environments more variable, or requiring the user to input shop and password for this command only without it being able to be loaded from environment or config.

@thisislawatts
Copy link
Contributor

@tanema 馃憤 on this being marked as a breaking change in the versioning on this software.

@andyw8
Copy link
Contributor

andyw8 commented Jul 27, 2020

Should this also have a changelog entry?

@tanema
Copy link
Contributor Author

tanema commented Jul 27, 2020

We can update the changelog when we do a release.

@tanema tanema merged commit c080e53 into master Jul 27, 2020
@tanema tanema deleted the make_tight_theme_id_constraint branch July 27, 2020 15:44
@PaulNewton
Copy link

@tanema Does this apply to manual commands or config env's too?
So even for production env's in the config, --allow-live still must be passed when using that env?
Or only when using the --theme-id parameter on the cli without an env?

But is not needed on readonly product env's? so we can still get away with "live" on sync configs

Theme IDs are strictly required. There is an error if the theme ID is blank.

I'm assuming that means if theme ID is any non-ID , so generating boilerplate configs with "live" will fail with this change?

@tanema
Copy link
Contributor Author

tanema commented Jul 27, 2020

@PaulNewton

So even for production env's in the config, --allow-live still must be passed when using that env?
The --allow-live flag will only apply to the production, This is specifically making it hard to make changes to the live theme so that less errors happen.

I have removed the theme_id: 'live' support. The live keyword will no longer be supported, nor will it default to the production theme if the theme_id is left out.

I'm assuming that means if theme ID is any non-ID , so generating boilerplate configs with "live" will fail with this change?

This is correct. We want to move toward a more strict environment where it is harder to change the live theme. theme get --list is still supported so you can still query your themes, with their IDs so this should not be too much of an inconvenience.

@PaulNewton
Copy link

get should probably echo back the environment(s) it's changing now that slightly more stuff may be going through it.

future visitor, themekit configure -t=123456789012 -e=envname will update IDs in existing environment's as will using the get command( ctrl+c to stop the download in most cli's)

Example flow after initial configure
theme get --list

  [123456789012][live] Theme #1
  [123456789023] Theme #1 Backup
  [098765432112] [DEV] Theme new feature

Then using get download files & the default behavior to overwrite theme_id for the development environment in config.yml
theme get -t=123456789012

Then to update a pre-existing production environment entry and download files
theme get -t=123456789012 --e=production
otherwise run configure again with -e= to make it

Before this change when manually running configure to create a default development env , I'd then just append boilerplate text of production env whose themeid was preset to live and just update the password in the file since other settings like ignore_files or timeout also need tweaking per store. This is partially because starting with a boilerplate config.yml that has production already is moot since passing multiple environments to configure spreads attributes like ignores_files into all passed in env's. So themekit configure -t=123456789012 -e=development -e=production could just lead to a situation where i'm unknowingly ignoring files, such as not syncing a merchants latest settings 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
4 participants