-
Couldn't load subscription status.
- Fork 9
Release the recent changes #914
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
base: main
Are you sure you want to change the base?
Conversation
Support building against the current 2025.3 EAP
…nto separate package
…of a Splunk reporter
…configure splunk telemetry
…metry reporters to forcefully enable telemetry. Only the Splunk telemetry reporter overrides the telemetry user setting.
|
@dividedmind The change to the version is only committed to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is just a merge of develop into main without further changes, then this is fine.
I briefly looked at the changes, but did not check all the changes.
Yeah, but this cumbersome and leads to those ugly back and forth merges. I just wonder if there is a better way. |
Well, we could drop |
Yeah, maybe that's a good idea. It might lead to a bit of a version churn, but I think that's okay. appmap-js has more churn and it releases directly — even to external repositories, unlike here where the marketplace releases are manual. @hleb-rubanau can you take a look and do sanity check whether there's anything we need to do besides just starting to merge to main instead of develop? |
I think that’s absolutely fine, especially since publishing is handled manually. The only thing worth keeping in mind is tracking published stable versions, and being ready (if development ever becomes nonlinear) to branch off dedicated “stable” lines for hotfixes. For example, if you need to patch an already released version while main has moved far ahead. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dividedmind from my side I don’t see any obstacles to approval, as long as you can confirm that hardcoding the instrumentation key is acceptable.
| class AppInsightsClient { | ||
| public class AppInsightsTelemetryReporter implements TelemetryReporter { | ||
| private static final @NotNull String EVENT_PREFIX = "appland.appmap/"; | ||
| private static final @NotNull String INSTRUMENTATION_KEY = "50c1a5c2-49b4-4913-b7cc-86a78d40745f"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dividedmind I’m not sure if it’s okay to hardcode the instrumentation key like this?
I understand there may be legitimate reasons for this choice, but even so,
maybe adding a brief comment explaining it could be helpful to avoid potential confusion.
| @NotNull final Integer version = 2; | ||
| @SuppressWarnings("unused") | ||
| class BaseData { | ||
| @SerializedName("ver") final int version = 2; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpicking: this hardcoded value 2 might be clearer as a package-level constant, even if it never changes and is required by an external consumer.
Let's package up the recent changes.
(BTW, is there a sane and easy way to make it so the plugin version in develop is current? It's still stuck at 0.67.1.)