Skip to content

Change pinecone binary to pc, style auth redirect landing pages - #25

Merged
austin-denoble merged 9 commits into
mainfrom
adenoble/update-pinecone-to-pc
Jul 17, 2025
Merged

Change pinecone binary to pc, style auth redirect landing pages#25
austin-denoble merged 9 commits into
mainfrom
adenoble/update-pinecone-to-pc

Conversation

@austin-denoble

@austin-denoble austin-denoble commented Jul 15, 2025

Copy link
Copy Markdown
Collaborator

Problem

I kept the custom redirect landing pages for authentication pretty basic in the CLI. We'd like to add some branding and color to these so they pop a bit more.

We've also had requests to change the pinecone command to pc so the user would go from calling pinecone login to calling pc login. This requires updating how the binaries are built through goreleaser, and making sure anyone currently using pinecone isn't immediately broken upon upgrading.

Solution

Redirect Pages

Thanks @cboltt for pointing out specific spots I should be looking. Most of these styles are lifted from the management console.

  • Add a bunch of custom CSS to both redirect_succes.html and redirect_error.html.
  • Add pinecone-logo.svg to assets and embed within the build like we do for the html pages.
  • In login.go we add the svg via simple text replacement when rendering the HTML.
Before After
Success Screenshot 2025-07-01 at 4 50 05 PM Screenshot 2025-07-15 at 3 50 37 PM
Failure Screenshot 2025-07-01 at 4 50 17 PM Screenshot 2025-07-15 at 3 54 32 PM

pinecone -> pc rename

Renaming the command name and executable took a bit of finessing. I needed to update our .goreleaser.yaml config to handle building the binaries as "pc" rather than "pinecone". The goal here was to not break or change the brew installation flow, so the name "pinecone" is still how you interact with the CLI via brew.

  • Update .goreleaser.yaml to update project_name and binary name to "pc". project_name needs to be updated as well because there's a limitation in goreleaser universal_binaries where it uses the project_name rather than the binary name when stitching things together, and we want to keep the naming consistent across all the executables, etc (I think).
  • Update all the documentation and Cobra command related strings from "pinecone" to "pc".
  • Add a brew install step in .goreleaser.yaml to create a symlink of "pinecone"->"pc". This is primarily to keep this update from hard-breaking anyone that might be using the CLI already, which is maybe unlikely, but this is an easy convenience.
  • At the top level of the CLI, add a small Fprint warning to let users know "pinecone" is deprecated and they should migrate to "pc".

Type of Change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update
  • Infrastructure change (CI configs, etc)
  • Non-code change (docs, etc)
  • None of the above: Styling / look & feel changes

Test Plan

To test locally, you'll need to pull this branch down and run goreleaser, and then run off the executable directly. It will be "pc" rather than "pinecone":

goreleaser build --single-target --snapshot --clean

./dist/pc_darwin_arm64/pc --help
./dist/pc_darwin_arm64/pc login
# etc, commands should all function as before, but the documentation output and everything should read as "pc"

I tested locally by manually building the pinecone.rb file for brew, and then installing from source. This required updating the pinecone.rb file to point to the local binary rather than remote, and updating the checksum:

  • Change pinecone.rb url field to local file reference: url "https://github.com/pinecone-io/cli/releases/download/v0.0.54/pc_Darwin_all.tar.gz" -> url "file://#{Dir.pwd}/dist/pc_Darwin_all.tar.gz"
  • regenerate sha256 and update that as well if necessary: shasum -a 256 dist/pc_Darwin_all.tar.gz
brew install --build-from-source /Users/austin/workspace/cli/dist/homebrew/Formula/pinecone.rb

I then tested the pc <-> pinecone symlink, and the warning output. It seems like this upgrade flow works fine from within brew, and you could test the above yourself if you wanted.

For testing the auth redirect page, initiate pc login and take a look at the success page after you auth.


@austin-denoble austin-denoble changed the title Update authentication redirect pages Change pinecone binary to pc, style auth redirect landing pages Jul 16, 2025
Comment thread .github/workflows/ci.yaml Fixed
Comment thread .github/workflows/ci.yaml Fixed
@austin-denoble
austin-denoble requested review from adit99 and Copilot July 16, 2025 20:48

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull Request Overview

This PR renames the CLI command from pinecone to pc and styles the authentication redirect landing pages with improved branding and visual design. It implements backward compatibility through symlinks and deprecation warnings.

  • Updates CLI command name from pinecone to pc across all files and documentation
  • Enhances authentication redirect pages with custom CSS styling, colors, and Pinecone branding
  • Maintains backward compatibility by creating symlinks and showing deprecation warnings

Reviewed Changes

Copilot reviewed 31 out of 32 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
cmd/pc/main.go New main entry point with deprecation warning for old command name
cmd/pinecone/main.go Removed old main entry point
.goreleaser.yaml Updated build configuration to use "pc" binary name with brew symlink
internal/pkg/cli/command/login/assets/* Enhanced HTML templates with modern CSS styling and branding
internal/pkg/cli/command/login/login.go Added SVG logo embedding and template replacement
internal/pkg/cli/command/root/root.go Updated root command configuration from "pinecone" to "pc"
Multiple command files Updated all command references and help text to use "pc" instead of "pinecone"
README.md, CONTRIBUTING.md Updated documentation to reflect new command name

Comment thread internal/pkg/cli/command/login/login.go Outdated
Comment thread internal/pkg/cli/command/login/login.go Outdated
Comment thread internal/pkg/cli/command/login/login.go Outdated
Comment thread internal/pkg/cli/command/login/assets/redirect_success.html Outdated
Comment thread internal/pkg/cli/command/login/assets/redirect_error.html Outdated
Comment thread .github/workflows/publish.yaml Fixed
Comment thread .github/workflows/ci.yaml
retention-days: 7
compression-level: 0
overwrite: true
tests:

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

There's not actually that much changing in these workflow files, there was just extra indention I wanted to back out.

@claude

claude Bot commented Jul 17, 2025

Copy link
Copy Markdown

Claude finished @austin-denoble's task —— View job


PR Review: Change pinecone binary to pc, style auth redirect landing pages

Review Progress:

  • Analyze CLI rename implementation
  • Review auth page styling changes
  • Check security considerations
  • Evaluate performance impact
  • Assess test coverage
  • Provide comprehensive feedback

Summary

This PR successfully implements two major changes: renaming the CLI from pinecone to pc and enhancing the authentication redirect pages with modern styling. The implementation is generally well-done with good backward compatibility considerations.

✅ Strengths

CLI Rename Implementation

  • Excellent backward compatibility: The goreleaser configuration correctly creates a symlink from pinecone to pc via bin.install_symlink "pc" => "pinecone" in .goreleaser.yaml:89
  • Clear deprecation strategy: The deprecation warning in cmd/pc/main.go:17 is user-friendly and informative
  • Comprehensive rename: All documentation, command references, and help text have been systematically updated
  • Consistent naming: The project_name update in goreleaser ensures consistent naming across all artifacts

Auth Page Styling

  • Modern design: The CSS styling uses contemporary design patterns with proper flexbox layout and CSS custom properties
  • Consistent branding: Colors and fonts align with Pinecone's design system
  • Proper accessibility: Good color contrast and semantic HTML structure
  • Clean template approach: Using Go's html/template for safe templating

⚠️ Issues & Concerns

Security Considerations

  1. Template injection risk: The current string replacement approach in login.go:187 is potentially vulnerable:

    templateData := map[string]template.HTML{"LogoSVG": template.HTML(logoSVG)}

    Using template.HTML() bypasses XSS protection. Consider validating the SVG content or using a safer approach.

  2. External font loading: The HTML templates load fonts from Google Fonts (fonts.googleapis.com) which creates a dependency on external services and potential privacy concerns.

Performance Issues

  1. Template parsing inefficiency: In login.go:231, templates are parsed on every request:

    tmpl, err := template.New("auth-response").Parse(htmlTemplate)

    Consider pre-parsing templates at startup for better performance.

  2. Duplicated template logic: The renderHTML function is called twice with similar logic. Consider creating a helper function or using a template cache.

Code Quality

  1. Missing error handling: In login.go:199, the type assertion w.(http.Flusher).Flush() could panic if the ResponseWriter doesn't implement Flusher.

  2. Unused CSS variables: The HTML templates define several CSS variables that aren't used (e.g., --pinecone-blue, --link-color, --secondary-text-color).

  3. Hard-coded values: The server address 127.0.0.1:59049 in login.go:205 is hard-coded. Consider making it configurable.

Test Coverage

  1. No test files found: The repository appears to lack test files (**/*_test.go returns no results), which means this significant change lacks automated testing coverage.

  2. Manual testing only: The PR relies entirely on manual testing, which increases the risk of regressions.

🔧 Recommendations

High Priority

  1. Add tests: Create unit tests for the login functionality, especially the template rendering and deprecation warning logic.

  2. Fix security issues:

    • Validate SVG content before embedding
    • Consider self-hosting fonts or making external requests optional
    • Use safer template handling
  3. Improve error handling: Add proper error handling for the Flusher type assertion.

Medium Priority

  1. Optimize template parsing: Pre-parse templates at startup or implement template caching.

  2. Extract template helper: Create a reusable template rendering function to reduce code duplication.

  3. Clean up CSS: Remove unused CSS variables or add comments explaining their purpose.

Low Priority

  1. Configuration: Make the callback server address configurable rather than hard-coded.

  2. Documentation: Add inline comments explaining the template replacement strategy.

📋 Files Requiring Attention

  • internal/pkg/cli/command/login/login.go:187-244 - Security and performance improvements needed
  • internal/pkg/cli/command/login/assets/*.html - Consider font loading strategy
  • cmd/pc/main.go:16-18 - Consider adding tests for deprecation warning

Overall Assessment

This is a solid PR that successfully implements the requested changes with good attention to backward compatibility. The main concerns are around security hardening, performance optimization, and test coverage. The breaking change is well-handled, and the styling improvements significantly enhance the user experience.

Recommendation: Approve with requested changes to address security concerns and add basic test coverage.

@austin-denoble
austin-denoble merged commit 0208a76 into main Jul 17, 2025
15 checks passed
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.

3 participants