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.

Sign up for GitHub

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

Jump to bottom

refactor: prepare for 10.7.0 release #1097

Merged
merged 3 commits into from Nov 30, 2023
Merged

refactor: prepare for 10.7.0 release #1097

merged 3 commits into from Nov 30, 2023

Conversation

Integralist
Copy link
Collaborator

No description provided.

@Integralist Integralist added the clean-up Code clean-up or refactor label Nov 27, 2023
Integralist
Integralist commented Nov 27, 2023
View reviewed changes
pkg/app/run.go
if slices.Contains(data.Args, "--metadata-enable") && !metadataDisable && !data.Config.CLI.MetadataNoticeDisplayed && commandCollectsData(commandName) {
text.Important(data.Output, "The Fastly CLI is configured to collect data related to Wasm builds (e.g. compilation times, resource usage, and other non-identifying data). To learn more about our data & privacy policies visit https://www.fastly.com/trust. Join the conversation https://bit.ly/wasm-metadata")
if !slices.Contains(data.Args, "--metadata-disable") && !metadataDisable && !data.Config.CLI.MetadataNoticeDisplayed && commandCollectsData(commandName) {
text.Important(data.Output, "The Fastly CLI is configured to collect data related to Wasm builds (e.g. compilation times, resource usage, and other non-identifying data). To learn more about what data is being collected, why, and how to disable it: https://developer.fastly.com/reference/cli/")
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@jaskiratr you'll need to ensure the DevHub has the relevant content there ready to go once we publish the new release.

Copy link

@jaskiratr jaskiratr Nov 28, 2023 •
edited by Integralist

Choose a reason for hiding this comment

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

I created a PR for the DevHub content.
I can also create a separate PR to take down the the Important notice on these pages https://developer.fastly.com/reference/cli/
https://developer.fastly.com/learning/tools/cli

Copy link

@jaskiratr jaskiratr Nov 28, 2023 •
edited by Integralist

Choose a reason for hiding this comment

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

Created a PR for removing the Important notice from the CLI pages.

@fgsch
Copy link
Member

fgsch commented Nov 28, 2023 •
edited

Code wise this looks good to me.
My only comment is that running fastly compute metadata outputs:

/fastly compute metadata
SUCCESS: configuration updated (see: `fastly config`)

Build Information: enable
Machine Information: disable
Package Information: enable

Which feels a bit strange -- is it printing the current settings, updating them to enable the build and package information, something else? why is it saying the configuration was updated? -- but perhaps it is aligned with other subcommands.

Also, I'd like the (optional) flags to be more prominent; this behaviour is highly contentious so IMO things need to be very evident and explicit.

@Integralist Integralist force-pushed the integralist/prep-10.7.0-release branch from 25b46ab to 058901a Compare November 28, 2023 11:47
@Integralist Integralist force-pushed the integralist/prep-10.7.0-release branch from dfb7a1b to a4dbd87 Compare November 29, 2023 10:02
@Integralist
Copy link
Collaborator Author

@fgsch I've resolved the issue you mentioned. Thanks for raising that!

@jaskiratr I'm going to add an option for disabling the script info data collection (e.g. the [scripts] section of the toml). Because if we just use fastly compute metadata --disable then yes none of the specific [wasm-metadata] fields (build, machine, package) are recorded, but we still get the build_script, env_vars, post_* script data recorded and that's misleading (yeah the user can use the one time --metadata-disable flag on compute build to disable everything but if they want to permanently disable data collection we need that to be handled by compute metadata --disable).

@Integralist
Copy link
Collaborator Author

OK I've pushed up changes to support disabling script_info. I think this PR is good to be merged.

@Integralist Integralist merged commit 4f952c8 into main Nov 30, 2023
6 checks passed
@Integralist Integralist deleted the integralist/prep-10.7.0-release branch November 30, 2023 10:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fgsch fgsch Awaiting requested review from fgsch

@jaskiratr jaskiratr Awaiting requested review from jaskiratr

Assignees
No one assigned
Labels
clean-up Code clean-up or refactor
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@Integralist @fgsch @jaskiratr