Celo integration rebase 15 experiment#347
Closed
philippecamacho wants to merge 2654 commits intodevelopfrom
Closed
Celo integration rebase 15 experiment#347philippecamacho wants to merge 2654 commits intodevelopfrom
philippecamacho wants to merge 2654 commits intodevelopfrom
Conversation
contracts: Update snapshots Update semver-lock
* contracts: scripts for L2 token deployment/verification See celo-org/celo-blockchain-planning#982 * contracts: Automatically read decimals for L2 tokens contracts: Convert `gen_l2_token_cmds` to std sh (#392) Changed shebang from #!/usr/bin/env fish to #!/bin/sh Replaced $argv with "$@" for command line arguments Replaced [ -z "$argv" ] with [ -z "$1" ] to check for arguments Replaced status filename with basename "$0" to get script name Changed return to exit 1 for script termination Used standard variable assignment and export syntax Used "$@" in the for loop to properly handle arguments with spaces Used $(...) for command substitution (POSIX compliant)
gomod: Fix build after op-geth update Remove fee currency context from call of NewEVMBlockContext
These clash with the check in celo-org/op-geth@2a740e5 and are safe to ignore, since the regolith fork is always enabled on Cel2 chains, so that we'll never migrate to it.
op-e2e: Enable Cel2 in e2e tests
EigenDA current limit for Holesky (their documentation is currently outdated but the limit seems to be set to 16 MB based on the updated tests from [this PR](Layr-Labs/eigenda-proxy#100)).
…ode startup (#367) * Set L1 safe and finalized head at startup of op-node * Wrap initialization of L1 safe & finalized head in SyncStatus with if block * Fix comment * Fix commen * Fix comment * Fix codes based on feedback * Fix comment * Swap the order of fetching finalized and safe L1 block references * Move L1 safe and finalized head fetching to the beginning of OpNode::Start * Remove unnecessary empty line * Add log in finalized
* feat(batcher): multi-frame altda channels * docs(batcher): add documentation for DaType and txData.daType * docs: fix NextTxData comment --------- Co-authored-by: Samuel Laferriere <samlaf92@gmail.com>
* test(altda): add test for altda->ethda failover * feat(batcher): altda->ethda failover when altda is down * chore: fix typos * fix(fakeDAServer): handlePut was still handling put when in failover mode * Fix logs --------- Co-authored-by: Samuel Laferriere <samlaf92@gmail.com>
* Add Prometheus metrics for AltDA failover in Batcher * Fix calls of RecordBatchDaType * Revert deleted comment * Add metrics for total stored batch size * Fix condition for RecordBatchDaType and RecordBatchDataSizeBytes * Add missing namespace to prometheus definition * Fix the amount of batch sizes recorded for DA * Unify recordings of size of batch to be stored * Improve Prometheus help text
* test(altda): add a test to make sure altda node keeps finalizing even after failover to ethda Currently it does not, as shown by the test TestAltDA_FinalizationAfterEthDAFailover failing * fix(damgr): ethda failover finalization stall bug Weiwei from Polymer found this bug. He proposed a solution. This is an alternative solution which seems simpler, but not 100% of its soundness. * fix: damgr_test doesn't compile * chore: add more logs to damgr and altda_data_source * docs(altda_test): fix typo --------- Co-authored-by: Samuel Laferriere <samlaf92@gmail.com>
… holocene order (#379) * fix(batcher): altda parallel submitted blobs respect strict holocene order (#21) test(e2e): new altda e2e test for concurrent blob submissions to maintain new holocene strict ordering rules test(batcher): add altda unit tests for unhappy failure cases (except channel timeout) test(batcher): fix flaky driver tests + speed them up test(batcher): robustify batcher driver altda tests fix(batcher): altda concurrent blob responses are reordered to respect holocene strict ordering rules docs: fix typos and add documentation comments for some batcher public methods test(op-alt-da): fix MockDAClient.DeleteData decrement semantic chore(batcher): move channel failover behavior from TxFailed to AltDASubmissionFailed The failover logic in the failover feature commits was aded on TxFailed, as the separate function AltDASubmissionFailed didn't exist yet. This change makes it much cleaner as a tx having failed cannot lead to a failover given... since that would come from an ethereum issue. fix(batcher): bug in sendTransaction chore(test-logger): fix test logger.Crit which wasn't getting flushed test(batcher): fix altDASetup w new channel config DaType was added, so needed to change config to use DaTypeAltDA style: wrap errors style(damock): dont explicitly initialize to 0 We make use default values instead docs(batcher-test): document why channel timeout test is left unimplemented docs(batcher): fix todos in batcher readme chore: make lint-go-fix docs(batcher): fix readme typo * Change test to write a log instead of stdout --------- Co-authored-by: Samuel Laferriere <samlaf92@gmail.com>
Add default to switch for lint Allow to override with flags the celo forks 22nd Jan 2026: Noted that we didn't need to set the PectraBlobScheduleTime flag, but now we do need to keep it as removing it would be a hardfork.
* isthmus: Update L1Block contract bytecode --------- Co-authored-by: Gastón Ponti <gaston.ponti@clabs.co> Co-authored-by: Gaston Ponti <pontigaston@gmail.com>
It is also prepared for using the bridged WETH as fee currency, but we are currently lacking a simple way to send fee currency txs, so I left the final tx out.
This uses the default fee-currency-directory address from op-geth. It will fix the issue that the EVM calls into the directoy will fail when executed on the local devnet. e2e: use `--broadcast` with `forge create` (#281) Forge started to require the `--broadcast` flag for actually deploying a contract. Otherwise it will only do a dry-run. We should really pin our foundry version. But let's wait until we rebase to the latest upstream, since there have been changes to the overall setup. Closes celo-org#278
These tests perform a check at the end to ensure that the total funds after a test match the total funds before the test. We had modified the state transition function to direct baseFee payments to the fee handler instead of optimism's OperatorFeeVault when in a cel2 context. This caused the tests to fail because the tests were not including the balance of the fee handler. This change ensures that we do consider the fee handler balance when calculating the total.
The test was configured with MaxFrameSize: 150 which was too small for the compressed block data (~291 bytes), causing 2 frames per block instead of 1. This doubled the AltDA Store count from the expected 5 to 10. Fixed by increasing MaxFrameSize to 400 to ensure each block fits in a single frame as the test intended.
Co-authored-by: Cursor <cursoragent@cursor.com>
Implement getting AWS Nitro attestations and commitmen signing in the batcher
* Add code sync procedure * Update links * Fix format * Rename files
* update batchAuthenticator according to audit report * gen bindings and fix fast-tests
* Simplify the test as we cannot in practice reduce the window size. --------- Co-authored-by: Philippe Camacho <philippe@espressosys.com>
* remove warning on every failed tx * reorder the checks
* Fallback mechanism test * Update op-e2e/system/e2esys/setup.go Co-authored-by: Phil <philippe@espressosys.com> --------- Co-authored-by: Philippe Camacho <philippe@espressosys.com>
* Address flakiness. * Simplify the code * Fix CI --------- Co-authored-by: Keyao Shen <shenkeyao@gmail.com>
* update single run-enclave.sh * remove BATCHER_PRIVATE_KEY * update run-enclave.sh
* Fallback recovery * Add caff node * Suggestions
* Make Attestation Verifier Service optional When the Attestation Verifier Service was added to the integration it fundamentally modified the testing experience, requiring external environment variables to be populated in order to run the tests. Additionally, these environment variable requirements were not documented in the README_ESPRESSO.md file for reference. This change modifies the Attestation Verifier Service setup for the E2E testing environment to make it opt-in instead of being forced to be enabled. Additionally, the Verifier URL is no longer required to run the Batcher. This is a double-edged sword, however, as it means that we could potentially deploy the service without the configuration, and we would potentially be lacking the registered attestation. This may be resolvable with a slight modification to the service configuration, that we would ultimately disable for the E2E testing environment. * Fix misspelling Fix linting error that has caught a misspelling of the work 'Network'. * Modify configuration address to be required from CLI With the change of making the Espresso Attestation Service optional we removed the CLI configuration check that occurs on launch, so that the E2E tests can still be run. This has an unfortunate side-effect of allowing the Batcher to be launched in a state where it is unable to operate as intended due to user error. The only indication being a `WARN` log entry to inform him/her of his/her mistake. This sort of approach is generally discouraged, yet we still need to be able to bypass this check for testing purposes. As a result the `EspressoAttestationService` value has been modified from being a simple `string` to being an interface whose value is inspectable and not allowed to be empty by default. This allows for the test configurations to overwrite this behavior, and allow an optional value in the cases where it is needed. This should preserve the prior behavior of erroring on launch when the parameter is not configured or specified, and should also preserve the new behavior where it is explicitly disabled in tests. * Fix some nil references The EspressoAttestationService configuration value being an interface makes it a `nil`lable value by default. Care needs to be taken when accessing this value an referencing it. This change adds some additional care in referencing the value stored within. * Fix nil access error The `l1Client` being created assumes that the `sys` returned from the call is non-nil before checking the error. This is not guaranteed, and is most likley not ever the case. As a result there is a potential for an error do to attempting an access on a `nil` value. By moving the `l1Client` declaration after the error check, we avoid the potential for this issue. * Apply linting and formatting changes * Fix e2e tests - populate default EspressoAttestationService With the modification of the EspressoAttestationService to an interface instead of an individual value, we need to ensure that the default way of launching the Espresso E2E DevNet results in the value being populated with an empty allowed value. This still allows for extension and override, without requiring the value to be specified, which is our intention. This was missed when adding the capability originally. * Cleanup code practices We have duplicated code that makes the maintenance burden more difficult than it needs to be. In many of these scenarios the code that is duplicated differs by only a single line. Instead of making the system more flexible, we ended up duplicating code paths. This increases the maintenance burden by needing to ensure that these code paths match in every case where they do not differ, yet they are independent of each other. This is not a great approach. Additionally, we end up with multiple starting points for something that should not need them. We also end up storing a configuration that is unnecessary to store. This incurs conditional checks where some are not needed, and ends up making the approach be more confusing than it needs to be. This change aims to replace these approaches with one that adheres to the functional option approach and preserves the existing behavior. * Revert EspressoAttestionService to a `string` As it so happens we rely on the `CLIConfig` for `Espresso`, and the `Batcher` to be serializable. By utilizing an `interface`, we run into trouble doing this. Due this constraint, the `interface` constraint is not feasible. This change reverts the value back to a `string`, which should result in a smaller overall change. It also opts for a private configuration value that is inspectable by the `Verify` check, but not directly configurable. We expose a method to allow for it to be configured, so it can only occur within code within the code base itself. We should only invoke this via Testing where we need the value to be optional. This achieves the same result but in a different way. | NOTE: There may be a better approach to this as well, isntead of having this be a separate field, we could do something akin to sql.NullString, where we encode this value as a Marshable `struct`. The acess pattern becomes different, but we could directly encode the empty allowance into the struct itself. * Add Espresso Attestation Verifier Service to Enclave Test The Enclave tests are currently failing in CI. It is dying due to an error stemming from the lack of the EspressoAttestationService being configured. It is likely that this is required for the Enclave tests specifically. As a result, we need to add and enable it for the enclave tests. * Modify LaunchBatcherInEnclave option The LaunchBatcherInEnclave essentially launches the batcher externally within an enclave. This option actually relies on the Espresso Attestation Verifier Service to be running. This is due to the Espresso Attestation Service only being optional inside of a test environment. When launched externally, the Batcher is no longer considered to be in a "test environment", or configurable for testing. As a result, its configuration **MUST** be something that can actually be resolvable from a CLI launch. Since the Espresso Attestation Verifier Service check is only disabled within the testing environment, this means it **MUST** be enabled in the enclave. For convenience, this option has been added automatically as a part of the LaunchBatcherInEnclave option, since it depends on it. This will minimize accidentl misconfigurations.
* add eigenda_proxy_url to op-batcher-tee * fix the url to post to eigenDA * not hardcoding EIGENDA_PROXY_PORT * fix the block height config
* Add test to check end of channel fallback Asana task: https://app.asana.com/1/1208976916964769/project/1209976130071762/task/1211892212379885?focus=true We need a test to check the fallback Batcher behavior in the event that the Espresso Batcher is able to submit a partial Channel that is im progress. The specific scenario we want to test for is one concerning a multi-frame channel that has had at least part of the full channel submitted to the L1 by the Espresso Batcher, then no more. After which we swap to the Fallback Batcher, and we should be able to pick up the missed / incomplete channel, and complete the transactions. * Rename helper function to match naming pattern * Fix lint issue with not checking error result of wait.For * Commit work in progress multi frame channel efforts * Adjust settings to successfully trigger multi-frame channels After a mob programming session @quentinl was able to help identify a a specific combiniation of parameters to successfully and consistently trigger multi-frames within the Batcher. This condition is a necessary precusor to the test being attempted. This commit updates the test with the information necessary to trigger this condition and sets the necessary test criteria that we are aiming to achieve. * Perform some code cleanup This change does a few things: - Address linting issue causing CI failure - Adjusts some golang forloop usage to be more modern - Adjust function call signatures to remove unused variables * Fix bug tracking unsuccessful frames in test In the `TxManagerIntercept` there is a bug that appends the successful frames to the unsuccessful ones. While this bug isn't great in the information that it taints, it doesn't actually have the large of an impact on the test as a whole, as the resulting failure condition would be triggered regardless. This bug does affect the accurate tracking of failed frames which could be valuable information for inspection. * Update espresso/environment/e2e_helpers.go Co-authored-by: Phil <philippe@espressosys.com> * Replace Disable Batcher setting references There are a number of places in our testing setup where we are explicitly preventing the Batcher from starting on launch. Instead of rewriting this same option every time we want to use it, we should reference a built in option that we can reference continually. This allows for non-repeated code and improved documentation as to the point and purpose of this option. * Refactor custom wait in test There's a condition being waited on in the switch to fallback batcher test. This wait is useful, and can be reused between tests. But the wait itself is somewhat hiding it's intention by being inline defined within the test itself. We should pull this wait out so it can be easily used, and its intention / purpose can be more easily documented. * Cleanup code reuse in frame decoding When decoding frame information for one of the Batcher fallback tests, there are similar code paths taken that result in most of the code being reused. We should clean up this code reuse so that we don't repeat ourselves in order to avoid diverging logic. Additionally, it allows us to reduce the amount of code needing to be maintained, and more clearly document the intention of the code, and the consistency with how we perform this frame decoding process. * Relocate deferred stop calls The Stop calls should occur as close to the launch of the environment as possible. As a result, any deferred calls to Stop for the system or the Espresso Dev Node should occur as close to their occurence as possible. * Modify Initial L2Verif wait to be longer With the specific Frame and Channel settings being specified in the `TestFallbackMechanismIntegrationTestChannelNotClosed` test, the initial startup check for the L2 Verifier is failing. This is due to our settings requiring the Verifier process to take a bit longer than normal. In general, we want to give it more time, but the time frame for the failure is hard-coded in the `wait` function being utilized. While we **could** add a simple `time.Sleep`, and this would work, this is generally a bad appraoch as it just adds an unchecked delay. Instead, we opt to utilize a simple `retry` for up to `n` times. In this case, we only need to wait up to `3x` the normal time, so ensure that we perform at least `3` times. * Fix failure in Batcher Fallback test The TestFallbackMechanismIntegrationTestChannelNotClosed test fails locally without stopping, in spite of the overall time limit being specified on the test. After some troubleshooting and debugging, We were able to chase down the cause to be due to the `RunSimpleMultiTransactions`. It's unclear as to why this was causing the process to hang for as long as it was. It seemed to not be handling timeout errors well for some reason. Either way, we fority this helper by setting an explicit time limit on it, and referncing the context whenever we're performing channel operations. This should allow the channel operations themselves not to block and hang the test. After this modification we were able to determine that this process was failing due to insufficient gas being provided. For some reason when running the transactions through this mechanism, we require even more gas than we're normally need. This seems a bit odd, perhaps it has to do with the differences in the transaction construction. In any case, we up the gas being provided so that this becomes a non-issue. * Fix linting issues * Update espresso/environment/tx_helpers.go Co-authored-by: Phil <philippe@espressosys.com> * Correct failure vs success in Send The triggered conditions for failures and successes are backwards in the `Send` method of `TxMangerIntercept`. Their specific frame markers should be switched. * Update espresso/environment/14_batcher_fallback_test.go Co-authored-by: Phil <philippe@espressosys.com> --------- Co-authored-by: Phil <philippe@espressosys.com>
* Check if the batcher is active before publishing to L1/DA * fix readme lint * more lint fixes * check batcher contract * Fix endless warning * add batch authenticator address to rollup config * handle contract undeployed error * attempt test in CI * add test to CI * Revert "add test to CI" This reverts commit 2a9678a7298d130616a7fa5cea5e250978ccfbd3. * add test to CI * remove jg/ from branches * attempt to clean up and make the test more reliable * fix ci error WaitUntilSafe undefined * revert 07a82bf * Fix `anvil_setBalance` not found error * Simplify isActive check * add batcher-active-publish-only to devnet tests justfile * - simplify test, one less batcher switch - increase timeouts for devnet test * Cleaned up the code, raise tx waiting time to 60s * Brought back original timeouts * started fallback batcher up + lint fix docker compose file * Ensure that in Espresso mode the batch authenticator address is set. * Removing all changes to driver.go and the tests are still passing. --------- Co-authored-by: Philippe Camacho <philippe@espressosys.com>
* Remove pre authenticated batcher * fix test
* Support namespace range endpoint (cherry picked from commit a73f7b6) * fix buils (cherry picked from commit e46909b) * update docker image (cherry picked from commit 0774898) * fix streamer tests (cherry picked from commit f752aa2) * fix streamer tests (cherry picked from commit 168426e) * fix tests (cherry picked from commit b942c28) * fix tests (cherry picked from commit b96622c) * use docker instead of cargo to generate allocs.json (cherry picked from commit efee3ac) * fix readme * address comments * remove fetch api
…pdate (#337) * Enable upgradability * Fix fmt * Fix file name * Fix tests * Clean up tests * Force clean build * Add temp artifact verification * More verification for artifact verification * Fix build command * Fix artifact * Fix artifact * Fix script * Fix and simplify the script * Fix proxyAdmin as well * Add back verification workflow * Fix more workflows * Restore version * Use EspressoTEEVerifierMock * Fix TeeType conversion * Fix fmt * Fix enum conflict * Fix version in test * Fix byte requirement * Add error * Enable batcher address update * Fix typo and remove redundant tests * Fix owner * Fix test build * transfer owner * Fix more tests * Fix devnet test * Fix unused param * Fix ec2 test * Fix enclave test * Fix fmt * Fix circleCI * Fix fmt again * Cleanup * Move events and errors to interface * Fix build * Update proxy admin permission
* add description for TestBatcherSwitching * Update espresso/environment/14_batcher_fallback_test.go Co-authored-by: Keyao Shen <shenkeyao@gmail.com> * Update espresso/environment/14_batcher_fallback_test.go Co-authored-by: Keyao Shen <shenkeyao@gmail.com> * Update espresso/environment/14_batcher_fallback_test.go Co-authored-by: Keyao Shen <shenkeyao@gmail.com> * Update espresso/environment/14_batcher_fallback_test.go Co-authored-by: Keyao Shen <shenkeyao@gmail.com> * Update espresso/environment/14_batcher_fallback_test.go Co-authored-by: Phil <philippe@espressosys.com> * Update espresso/environment/14_batcher_fallback_test.go Co-authored-by: Phil <philippe@espressosys.com> --------- Co-authored-by: Keyao Shen <shenkeyao@gmail.com> Co-authored-by: Phil <philippe@espressosys.com>
* cmd to shutdown all services * Small change to trigger CI --------- Co-authored-by: Keyao Shen <shenkeyao@gmail.com>
Co-authored-by: OpenCode <noreply@opencode.ai>
Co-authored-by: Philippe Camacho <philippe@espressosys.com>
Summary of ChangesHello @philippecamacho, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly refactors and modernizes the project's CI/CD pipeline and MIPS VM infrastructure. It transitions the MIPS VM to a 64-bit architecture with an improved memory management system, expands syscall functionality, and introduces a more robust testing framework. These changes aim to enhance performance, maintainability, and code quality across the codebase. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Ignored Files
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request introduces a massive refactoring of the CI/CD pipeline, moving from machine executors to Docker, simplifying job definitions by using Makefiles, and adding new workflows for acceptance tests, issue management, and metrics collection. It also includes significant updates to the MIPS VM implementation, particularly around memory management and multi-threading, and removes 32-bit support in many places. My review focuses on the CI configuration and some of the core VM changes. I've identified a potential bug in the Discord notification script within the CircleCI configuration and an issue in the memory copying logic. Overall, the changes seem to align with modernizing the CI and evolving the VM architecture.
Comment on lines
+305
to
322
| func (m *Memory) Copy() *Memory { | ||
| pages := make(map[Word]*CachedPage) | ||
| table := m.merkleIndex.New(pages) | ||
| out := &Memory{ | ||
| merkleIndex: table, | ||
| pageTable: pages, | ||
| lastPageKeys: [2]Word{^Word(0), ^Word(0)}, // default to invalid keys, to not match any pages | ||
| lastPage: [2]*CachedPage{nil, nil}, | ||
| } | ||
|
|
||
| p, ok := m.pageLookup(pageIndex) | ||
| if !ok { | ||
| p = m.AllocPage(pageIndex) | ||
| } | ||
| p.InvalidateFull() | ||
| copy(p.Data[pageAddr:], chunk[:n]) | ||
| addr += Word(n) | ||
| for k, page := range m.pageTable { | ||
| data := make(Page, PageSize) | ||
| // *data = *page.Data | ||
| copy(data, page.Data) | ||
| out.AllocPage(k).Data = data | ||
| } | ||
| return out | ||
| } |
There was a problem hiding this comment.
The Copy() function does not copy the MappedRegions field from the source Memory object to the new one. The GetWord method depends on MappedRegions to read from memory-mapped regions. Without copying this slice, the new Memory object will have a nil MappedRegions slice and might behave incorrectly if it's expected to be a full copy with the same memory layout.
Suggested change
| func (m *Memory) Copy() *Memory { | |
| pages := make(map[Word]*CachedPage) | |
| table := m.merkleIndex.New(pages) | |
| out := &Memory{ | |
| merkleIndex: table, | |
| pageTable: pages, | |
| lastPageKeys: [2]Word{^Word(0), ^Word(0)}, // default to invalid keys, to not match any pages | |
| lastPage: [2]*CachedPage{nil, nil}, | |
| } | |
| p, ok := m.pageLookup(pageIndex) | |
| if !ok { | |
| p = m.AllocPage(pageIndex) | |
| } | |
| p.InvalidateFull() | |
| copy(p.Data[pageAddr:], chunk[:n]) | |
| addr += Word(n) | |
| for k, page := range m.pageTable { | |
| data := make(Page, PageSize) | |
| // *data = *page.Data | |
| copy(data, page.Data) | |
| out.AllocPage(k).Data = data | |
| } | |
| return out | |
| } | |
| func (m *Memory) Copy() *Memory { | |
| pages := make(map[Word]*CachedPage) | |
| table := m.merkleIndex.New(pages) | |
| out := &Memory{ | |
| merkleIndex: table, | |
| pageTable: pages, | |
| lastPageKeys: [2]Word{^Word(0), ^Word(0)}, // default to invalid keys, to not match any pages | |
| lastPage: [2]*CachedPage{nil, nil}, | |
| MappedRegions: m.MappedRegions, | |
| } | |
| for k, page := range m.pageTable { | |
| data := make(Page, PageSize) | |
| // *data = *page.Data | |
| copy(data, page.Data) | |
| out.AllocPage(k).Data = data | |
| } | |
| return out | |
| } |
Comment on lines
+203
to
+224
| command: | | ||
| if [ "${CIRCLE_BRANCH}" == "develop" ]; then | ||
| # Format message for Discord with better structure and formatting | ||
| DISCORD_MESSAGE="🚨 **CI Failure Detected** 🚨\n" | ||
| DISCORD_MESSAGE="${DISCORD_MESSAGE}> **Repository:** \`${CIRCLE_PROJECT_USERNAME}/${CIRCLE_PROJECT_REPONAME}\`\n" | ||
| DISCORD_MESSAGE="${DISCORD_MESSAGE}> **Branch:** \`${CIRCLE_BRANCH}\`\n" | ||
| DISCORD_MESSAGE="${DISCORD_MESSAGE}> **Job:** \`${CIRCLE_JOB}\`\n" | ||
| DISCORD_MESSAGE="${DISCORD_MESSAGE}> **Build Link:** ${CIRCLE_BUILD_URL}" | ||
|
|
||
| # Add failure reason if provided | ||
| if [ ! -z "<< parameters.message >>" ]; then | ||
| DISCORD_MESSAGE="${DISCORD_MESSAGE}\n\n**Failure message:** << parameters.message >>" | ||
| fi | ||
|
|
||
| # Add mentions if provided | ||
| if [ ! -z "<< parameters.mentions >>" ]; then | ||
| DISCORD_MESSAGE="${DISCORD_MESSAGE}\n\n**Attention:** << parameters.mentions >>" | ||
| fi | ||
|
|
||
| # Post to Discord webhook | ||
| curl -X POST -H "Content-Type: application/json" \ | ||
| -d "{\"content\": \"${DISCORD_MESSAGE}\"}" "${notify_ci}" |
There was a problem hiding this comment.
The way the DISCORD_MESSAGE is constructed and then used in the curl command will likely result in an invalid JSON payload. The \n sequences for newlines will be interpreted by the shell, but for the JSON payload sent with curl -d, they need to be escaped as \\n to be treated as literal newlines by Discord.
I suggest escaping the backslashes in your message strings to ensure the JSON payload is correctly formatted.
command: |
if [ "${CIRCLE_BRANCH}" == "develop" ]; then
# Format message for Discord with better structure and formatting
DISCORD_MESSAGE="🚨 **CI Failure Detected** 🚨\\n"
DISCORD_MESSAGE="${DISCORD_MESSAGE}> **Repository:** `${CIRCLE_PROJECT_USERNAME}/${CIRCLE_PROJECT_REPONAME}`\\n"
DISCORD_MESSAGE="${DISCORD_MESSAGE}> **Branch:** `${CIRCLE_BRANCH}`\\n"
DISCORD_MESSAGE="${DISCORD_MESSAGE}> **Job:** `${CIRCLE_JOB}`\\n"
DISCORD_MESSAGE="${DISCORD_MESSAGE}> **Build Link:** ${CIRCLE_BUILD_URL}"
# Add failure reason if provided
if [ ! -z "<< parameters.message >>" ]; then
DISCORD_MESSAGE="${DISCORD_MESSAGE}\\n\\n**Failure message:** << parameters.message >>"
fi
# Add mentions if provided
if [ ! -z "<< parameters.mentions >>" ]; then
DISCORD_MESSAGE="${DISCORD_MESSAGE}\\n\\n**Attention:** << parameters.mentions >>"
fi
# Post to Discord webhook
curl -X POST -H "Content-Type: application/json" \
-d "{\"content\": \"${DISCORD_MESSAGE}\"}" "${notify_ci}"
fi
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
18 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #<ISSUE_NUMBER>
This PR:
This PR does not:
Key places to review: