Skip to content

Pawel/new project token#422

Merged
pawelsz-rb merged 12 commits intomasterfrom
pawel/new_project_token
Apr 30, 2025
Merged

Pawel/new project token#422
pawelsz-rb merged 12 commits intomasterfrom
pawel/new_project_token

Conversation

@pawelsz-rb
Copy link
Copy Markdown
Collaborator

@pawelsz-rb pawelsz-rb commented Apr 14, 2025

Description of the change

Adding support for token version v2 and v3 (public_id)

Users will need to download new terraform plugin only when they are enabled for new token and when they want to create new tokens (because API's behavior has changed for new token CREATE ). If the users don’t want to create new tokens, then UPDATE, READ and DELETE will work with old terraform plugin (even when they are enabled for new tokens).

Note: we need to set the default value of token_type to legacy in order to be backward compatible. That way our users will need to specify token type while creating a new token, which is different from legacy

Type of change

  • Bug fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Maintenance
  • New release

Checklists

Development

  • Lint rules pass locally
  • The code changed/added as part of this pull request has been covered with tests
  • All tests related to the changed code pass in development

Code review

  • This pull request has a descriptive title and information useful to a reviewer. There may be a screenshot or screencast attached
  • "Ready for review" label attached to the PR and reviewers assigned
  • Issue from task tracker has a link to this pull request
  • Changes have been reviewed by at least one other engineer

@coveralls
Copy link
Copy Markdown

coveralls commented Apr 14, 2025

Pull Request Test Coverage Report for Build 14724865123

Details

  • 1 of 6 (16.67%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.3%) to 91.816%

Changes Missing Coverage Covered Lines Changed/Added Lines %
client/project_access_token.go 1 6 16.67%
Totals Coverage Status
Change from base Build 13541355893: -0.3%
Covered Lines: 1481
Relevant Lines: 1613

💛 - Coveralls

@pawelsz-rb pawelsz-rb requested review from brianr and Copilot April 14, 2025 09:18
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot reviewed 9 out of 9 changed files in this pull request and generated no comments.

@pawelsz-rb pawelsz-rb requested a review from waltjones April 14, 2025 09:22
@papi1992
Copy link
Copy Markdown

Hi Pawel,

I'm not an experienced golang developer, neither a terraform one, but looked thru the code and it looks good to me but discussing the changes with my team members one question raised.
Is it possible to avoid the accessToken = publicId correspond in the codebase?
there are 3 references => 1, 2, 3

@pawelsz-rb
Copy link
Copy Markdown
Collaborator Author

Hi Pawel,

I'm not an experienced golang developer, neither a terraform one, but looked thru the code and it looks good to me but discussing the changes with my team members one question raised. Is it possible to avoid the accessToken = publicId correspond in the codebase? there are 3 references => 1, 2, 3

well, it's implemented according to how the api was written, at least when I tested it. I am not changing any POST parameter and assign access_token to public id for UPDATE, READ and DELETE . I can change it to token = publicID instead

Comment on lines 191 to 199
accessToken := d.Get("access_token").(string)
publicID := d.Get("public_id").(string)

if publicID != "" {
accessToken = publicID
}
projectID := d.Get("project_id").(int)
l := log.With().
Str("accessToken", accessToken).
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Suggested change
accessToken := d.Get("access_token").(string)
publicID := d.Get("public_id").(string)
if publicID != "" {
accessToken = publicID
}
projectID := d.Get("project_id").(int)
l := log.With().
Str("accessToken", accessToken).
accessToken := d.Get("access_token").(string)
publicID := d.Get("public_id").(string)
tokenIdentifier := publicID
if tokenIdentifier == "" {
tokenIdentifier = accessToken
}
projectID := d.Get("project_id").(int)
l := log.With().
Str("tokenIdentifier", tokenIdentifier).

and where it's used at line 206 >

pat, err := c.ReadProjectAccessToken(projectID, tokenIdentifier) // accessToken => tokenIdentifier

Copy link
Copy Markdown

@papi1992 papi1992 left a comment

Choose a reason for hiding this comment

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

👍

@pawelsz-rb pawelsz-rb merged commit c13adf4 into master Apr 30, 2025
21 checks passed
@pawelsz-rb pawelsz-rb deleted the pawel/new_project_token branch April 30, 2025 05:49
@awendt
Copy link
Copy Markdown

awendt commented Jun 3, 2025

@pawelsz-rb With our existing tokens, this change is forcing us to re-create all our tokens. From the Terraform plan:

  # rollbar_project_access_token.foo must be replaced
-/+ resource "rollbar_project_access_token" "foo" {
      ~ access_token                = (sensitive value)
      ~ cur_rate_limit_window_count = 0 -> (known after apply)
      ~ cur_rate_limit_window_start = 1703176242 -> (known after apply)
      ~ date_created                = 1703176242 -> (known after apply)
      ~ date_modified               = 1703176242 -> (known after apply)
      ~ id                          = "[redacted]" -> (known after apply)
        name                        = "foo"
      + public_id                   = (known after apply)
      + token_type                  = "legacy" # forces replacement
        # (5 unchanged attributes hidden)
    }

What is the migration path here if we don't want to replace all our existing tokens?

@pawelsz-rb
Copy link
Copy Markdown
Collaborator Author

@awendt , this should not happen when you just upgrade the terraform plugin without changing token info on UI. I have tested multiple scenarios and never ran to this problem. Are you enabled for new token type or you are still using old type?

@awendt
Copy link
Copy Markdown

awendt commented Jun 6, 2025

@pawelsz-rb Sorry for the confusion, this was due to me running terraform apply without refresh 🤦🏻‍♂️

@jtsaito
Copy link
Copy Markdown
Contributor

jtsaito commented Jul 2, 2025

@pawelsz-rb Could you please point to me to documentation on how to safely migrate our account that is currently using the legacy token to using the new project access type tokens?

Can we safely ask Rollbar support to enable the new feature without immediately changing existing Terraform ProjectAccessToken resources and then gradually migrate them one by one over time?

@pawelsz-rb
Copy link
Copy Markdown
Collaborator Author

@jtsaito , yes that should be the case, you can safely ask the support team to enable new feature, and if you don't touch your existing tokens everything should be transparent to you. Then, old tokens remain legacy tokens and you will be able to create only new tokens.

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.

6 participants